Development: Initialize new Hyperion module for programming exercise creation assistance and migrate functionality from Iris
#11047
Conversation
…nsistency checking
github-actions
bot
added
tests
server
WalkthroughThis change introduces a new AI-powered Hyperion module with gRPC-based services for reviewing and refining programming exercises, including consistency checking and problem statement rewriting. It removes the previous Iris/Pyris-based consistency check functionality, updates documentation and build configuration for gRPC integration, and adds new backend, frontend, and test code to support the Hyperion workflow. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant REST_API
participant HyperionService
participant gRPC_Hyperion
User->>Frontend: Initiate consistency check or rewrite
Frontend->>REST_API: POST /api/hyperion/review-and-refine/...
REST_API->>HyperionService: checkConsistency() or rewriteProblemStatement()
HyperionService->>gRPC_Hyperion: gRPC call (CheckInconsistencies/RewriteProblemStatement)
gRPC_Hyperion-->>HyperionService: gRPC response
HyperionService-->>REST_API: Result or error
REST_API-->>Frontend: HTTP response
Frontend-->>User: Show result or error
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
🧰 Additional context used📓 Path-based instructions (2)`src/main/java/**/*.java`: naming:CamelCase; principles:{single_responsibility,s...
⚙️ Source: CodeRabbit Configuration File List of files the instruction was applied to:
`src/test/java/**/*.java`: test_naming: descriptive; test_size: small_specific; ...
⚙️ Source: CodeRabbit Configuration File List of files the instruction was applied to:
🧠 Learnings (5)📓 Common learningssrc/main/java/de/tum/cit/aet/artemis/hyperion/service/AbstractHyperionGrpcService.java (1)src/main/java/de/tum/cit/aet/artemis/hyperion/config/HyperionHealthIndicator.java (4)src/test/java/de/tum/cit/aet/artemis/hyperion/TestReviewAndRefineService.java (6)src/test/java/de/tum/cit/aet/artemis/hyperion/HyperionReviewAndRefineIntegrationTest.java (18)🧬 Code Graph Analysis (1)src/test/java/de/tum/cit/aet/artemis/hyperion/TestReviewAndRefineService.java (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (5)
✨ Finishing Touches
🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/test/java/de/tum/cit/aet/artemis/hyperion/architecture/HyperioRepositoryArchitectureTest.java (1)
7-9: Add @OverRide annotation for method overriding.The
getModulePackage()method is overriding an abstract method from the parent class but is missing the@Overrideannotation, which is a Java best practice for method overriding.+ @Override public String getModulePackage() { return ARTEMIS_PACKAGE + ".hyperion"; }src/main/java/de/tum/cit/aet/artemis/hyperion/service/HyperionReviewAndRefineService.java (1)
155-158: Consider more specific exception handling.Catching all exceptions might mask unexpected errors. Consider catching specific expected exceptions (e.g.,
IOException) to better distinguish between expected and unexpected failures.src/main/java/de/tum/cit/aet/artemis/hyperion/config/HyperionConfigurationProperties.java (1)
96-103: Consider using StringBuilder for better performance.The current implementation uses string concatenation which can be inefficient. Consider using StringBuilder for better performance.
@Override public String toString() { return new StringBuilder("HyperionConfigurationProperties{") .append("host='").append(host).append('\'') .append(", port=").append(port) .append(", useTls=").append(useTls) .append(", clientCertPath='").append(clientCertPath != null && !clientCertPath.isEmpty() ? "[CONFIGURED]" : "not set").append('\'') .append(", clientKeyPath='").append(clientKeyPath != null && !clientKeyPath.isEmpty() ? "[CONFIGURED]" : "not set").append('\'') .append(", serverCaPath='").append(serverCaPath != null && !serverCaPath.isEmpty() ? "[CONFIGURED]" : "not set").append('\'') .append(", defaultTimeoutSeconds=").append(defaultTimeoutSeconds) .append(", consistencyCheckTimeoutSeconds=").append(consistencyCheckTimeoutSeconds) .append('}') .toString(); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
.github/issue-labeler.ymlis excluded by!**/*.yml.github/labeler.ymlis excluded by!**/*.yml.idea/runConfigurations/Artemis__Server__LocalVC___LocalCI__Hyperion.xmlis excluded by!**/*.xmlsrc/main/resources/config/application-artemis.ymlis excluded by!**/*.yml
📒 Files selected for processing (39)
.github/CODEOWNERS(3 hunks)README.md(2 hunks)build.gradle(2 hunks)docs/dev/development-process/development-process.rst(1 hunks)docs/dev/setup/server.rst(3 hunks)gradle.properties(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/hyperion/config/HyperionConfigurationProperties.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/hyperion/config/HyperionGrpcConfiguration.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/hyperion/config/HyperionHealthIndicator.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/hyperion/service/HyperionReviewAndRefineService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/hyperion/service/grpc/AbstractHyperionGrpcService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/hyperion/web/HyperionReviewAndRefineResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/IrisConsistencyCheckService.java(0 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisStatusUpdateService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/consistencyCheck/PyrisConsistencyCheckPipelineExecutionDTO.java(0 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/consistencyCheck/PyrisConsistencyCheckStatusUpdateDTO.java(0 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/job/ConsistencyCheckJob.java(0 hunks)src/main/java/de/tum/cit/aet/artemis/iris/web/IrisConsistencyCheckResource.java(0 hunks)src/main/java/de/tum/cit/aet/artemis/iris/web/open/PublicPyrisStatusUpdateResource.java(0 hunks)src/main/webapp/app/app.constants.ts(1 hunks)src/main/webapp/app/programming/manage/instructions-editor/programming-exercise-editable-instruction.component.ts(2 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/artemis-intelligence/artemis-intelligence.service.ts(2 hunks)src/main/webapp/i18n/de/health.json(1 hunks)src/main/webapp/i18n/en/health.json(1 hunks)src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java(0 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/AbstractHyperionIntegrationTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/HyperionReviewAndRefineIntegrationTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/architecture/HyperioRepositoryArchitectureTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/architecture/HyperionApiArchitectureTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/architecture/HyperionCodeStyleArchitectureTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/architecture/HyperionResourceArchitectureTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/architecture/HyperionServiceArchitectureTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/config/HyperionHealthIndicatorTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/config/HyperionMockServiceConfiguration.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/config/HyperionTestConfiguration.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/config/HyperionTestServerConfiguration.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/iris/PyrisConcistencyCheckIntegrationTest.java(0 hunks)src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationLocalCILocalVCTest.java(2 hunks)
💤 Files with no reviewable changes (8)
- src/test/java/de/tum/cit/aet/artemis/core/connector/IrisRequestMockProvider.java
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/consistencyCheck/PyrisConsistencyCheckStatusUpdateDTO.java
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/consistencyCheck/PyrisConsistencyCheckPipelineExecutionDTO.java
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/job/ConsistencyCheckJob.java
- src/main/java/de/tum/cit/aet/artemis/iris/web/open/PublicPyrisStatusUpdateResource.java
- src/main/java/de/tum/cit/aet/artemis/iris/web/IrisConsistencyCheckResource.java
- src/test/java/de/tum/cit/aet/artemis/iris/PyrisConcistencyCheckIntegrationTest.java
- src/main/java/de/tum/cit/aet/artemis/iris/service/IrisConsistencyCheckService.java
🧰 Additional context used
📓 Path-based instructions (4)
`src/main/webapp/i18n/de/**/*.json`: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/...
src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
src/main/webapp/i18n/de/health.json
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalC...
src/main/webapp/app/app.constants.tssrc/main/webapp/app/programming/manage/instructions-editor/programming-exercise-editable-instruction.component.tssrc/main/webapp/app/shared/monaco-editor/model/actions/artemis-intelligence/artemis-intelligence.service.ts
`src/test/java/**/*.java`: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_...
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationLocalCILocalVCTest.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/architecture/HyperionResourceArchitectureTest.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/architecture/HyperionServiceArchitectureTest.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/architecture/HyperionCodeStyleArchitectureTest.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/architecture/HyperioRepositoryArchitectureTest.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/architecture/HyperionApiArchitectureTest.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/config/HyperionHealthIndicatorTest.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/AbstractHyperionIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/config/HyperionTestConfiguration.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/config/HyperionTestServerConfiguration.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/HyperionReviewAndRefineIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/config/HyperionMockServiceConfiguration.java
`src/main/java/**/*.java`: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,de...
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.javasrc/main/java/de/tum/cit/aet/artemis/hyperion/config/HyperionHealthIndicator.javasrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisStatusUpdateService.javasrc/main/java/de/tum/cit/aet/artemis/hyperion/service/grpc/AbstractHyperionGrpcService.javasrc/main/java/de/tum/cit/aet/artemis/hyperion/service/HyperionReviewAndRefineService.javasrc/main/java/de/tum/cit/aet/artemis/hyperion/config/HyperionConfigurationProperties.javasrc/main/java/de/tum/cit/aet/artemis/hyperion/config/HyperionGrpcConfiguration.javasrc/main/java/de/tum/cit/aet/artemis/hyperion/web/HyperionReviewAndRefineResource.java
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: client-style
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: server-style
- GitHub Check: Analyse
🔇 Additional comments (42)
src/test/java/de/tum/cit/aet/artemis/hyperion/architecture/HyperioRepositoryArchitectureTest.java (1)
5-10: Architecture test implementation follows the established pattern.The test class correctly extends
AbstractModuleRepositoryArchitectureTestand specifies the hyperion module package, which aligns with the architectural testing strategy for the new Hyperion module integration.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisStatusUpdateService.java (2)
61-73: Constructor dependency removal aligns with consistency check migration.The constructor modification appears to remove the
IrisConsistencyCheckServicedependency, which is consistent with the migration from Iris to Hyperion for consistency check functionality. The constructor follows proper dependency injection patterns.
40-203: Verify that all consistency check references have been properly removed.While the visible changes look correct, ensure that all references to consistency check functionality have been completely removed from this service class, including any
handleStatusUpdatemethods forConsistencyCheckJobthat may have existed.#!/bin/bash # Description: Verify that no consistency check references remain in the codebase # Expected: No references to ConsistencyCheckJob or IrisConsistencyCheckService should be found echo "Searching for remaining consistency check references..." rg -i "ConsistencyCheckJob|IrisConsistencyCheckService" --type java -A 2 -B 2gradle.properties (1)
39-41: Addition of Hyperion version property looks correct.The new
# Edutelligencesection andhyperion_version=0.1.0-SNAPSHOTfollow the existing version property pattern. Ensure thathyperion_versionis referenced appropriately inbuild.gradleto pull in the Hyperion gRPC client..github/CODEOWNERS (3)
20-20: Add Hyperion server code owner.The new entry for
/src/main/java/de/tum/cit/aet/artemis/hyperioncorrectly assigns@ls1intum/hyperion-maintainersand precedes the Iris entry for proper matching.
33-33: Add Hyperion client code owner.The entry for
/src/main/webapp/app/hyperionaligns with the client-side integration and uses the same maintainers team.
47-47: Add Hyperion test code owner.Including
/src/test/java/de/tum/cit/aet/artemis/hyperionensures Hyperion tests are routed to the correct team.src/main/webapp/i18n/en/health.json (1)
29-31: Register Hyperion health indicator in English locale.Adding
"hyperion": "Hyperion"in theindicatorblock allows the UI to display the new Hyperion health status alongside existing checks.src/main/webapp/i18n/de/health.json (1)
29-31: Register Hyperion health indicator in German locale.The
"hyperion": "Hyperion"entry integrates the new health check. As it’s a service name, leaving it untranslated complies with the informal style guideline.docs/dev/development-process/development-process.rst (1)
121-121: Update PR naming tag list with Hyperion.Including
HyperionbetweenIrisandScalabilityaccurately reflects the new feature tag in the naming conventions.src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationLocalCILocalVCTest.java (2)
7-7: LGTM: Consistent import patternThe import follows the established pattern of static imports for profile constants from the Constants class.
79-79: LGTM: Proper profile integrationThe addition of
PROFILE_HYPERIONto the active profiles list ensures that Hyperion-related configurations and beans are properly activated during integration tests.src/main/webapp/app/app.constants.ts (1)
68-68: LGTM: Consistent profile constantThe frontend constant properly matches the backend
PROFILE_HYPERIONvalue and follows the established naming convention in this file.src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
321-324: LGTM: Well-documented profile constantThe new
PROFILE_HYPERIONconstant follows the established pattern with proper JavaDoc documentation and consistent naming convention.README.md (2)
76-76: LGTM: Clear service descriptionThe description accurately captures Hyperion's role as an AI-powered programming exercise creation assistance service within the EduTelligence suite.
149-149: LGTM: Proper maintainer assignmentFelix Dietrich is appropriately assigned as the maintainer for Hyperion, consistent with his existing responsibilities for modeling and text exercises.
src/test/java/de/tum/cit/aet/artemis/hyperion/architecture/HyperionServiceArchitectureTest.java (1)
1-10: LGTM: Standard architecture test implementationThe architecture test follows the established pattern for module-specific service tests, properly extending
AbstractModuleServiceArchitectureTestand returning the correct module package path.src/test/java/de/tum/cit/aet/artemis/hyperion/architecture/HyperionResourceArchitectureTest.java (1)
1-11: LGTM - Clean architecture test implementation.The class correctly extends the abstract base class and provides the required module package identifier for Hyperion resource architecture validation.
src/test/java/de/tum/cit/aet/artemis/hyperion/architecture/HyperionCodeStyleArchitectureTest.java (1)
1-17: LGTM - Appropriate code style architecture test setup.The class correctly implements the required methods with reasonable threshold configuration for a new module.
build.gradle (2)
453-456: LGTM - Consistent test dependency versions.The test dependencies correctly use the same gRPC version as the implementation dependencies, ensuring consistency.
404-410: Verify gRPC version compatibility and security.The gRPC dependencies are well-structured with consistent versioning. Please verify that version 1.71.0 is recent and free from known vulnerabilities.
What is the latest stable version of gRPC Java and are there any known security vulnerabilities in version 1.71.0?src/test/java/de/tum/cit/aet/artemis/hyperion/architecture/HyperionApiArchitectureTest.java (1)
1-19: LGTM - Correct API architecture test implementation.The class properly implements the required methods with no ignored classes, ensuring comprehensive architecture validation for the new Hyperion module.
src/test/java/de/tum/cit/aet/artemis/hyperion/config/HyperionHealthIndicatorTest.java (1)
1-36: LGTM - Well-structured health indicator test.The test follows all coding guidelines perfectly:
- Uses
assertThatfor assertions- Descriptive naming (
healthCheck_shouldReturnUp)- Small and specific scope
- Proper Spring Boot test configuration with Hyperion profile
- Validates both status and expected detail keys
The test appropriately validates the health indicator's core functionality.
src/test/java/de/tum/cit/aet/artemis/hyperion/AbstractHyperionIntegrationTest.java (1)
1-28: LGTM! Well-structured abstract test base class.The implementation correctly follows Spring test patterns with appropriate profile activation and service injection. The empty lifecycle methods with explanatory comments are good practice when no specific setup/teardown is required.
src/test/java/de/tum/cit/aet/artemis/hyperion/config/HyperionTestConfiguration.java (1)
1-32: LGTM! Proper test configuration for gRPC mocking.The configuration correctly uses
@TestConfigurationwith profile activation and@Primaryannotation to override production beans during tests. The in-process gRPC channel setup enables fast, reliable testing without network dependencies.docs/dev/setup/server.rst (3)
73-79: LGTM! Clear configuration documentation.The Hyperion configuration block provides appropriate default values and helpful comments for both development and production setups.
116-117: LGTM! Consistent documentation pattern.The run configuration documentation follows the established pattern for other services like Athena, making it easy for developers to understand.
263-311: LGTM! Comprehensive setup instructions.The Hyperion service section provides detailed, step-by-step instructions covering:
- External service deployment
- Development and production configuration
- Profile activation
- Health endpoint verification
The YAML examples are clear and include security considerations for production environments.
src/main/webapp/app/programming/manage/instructions-editor/programming-exercise-editable-instruction.component.ts (2)
44-44: LGTM! Consistent import pattern.Adding
PROFILE_HYPERIONalongsidePROFILE_IRISmaintains consistency while enabling the new Hyperion functionality.
89-99: LGTM! Improved architecture with computed properties.The refactoring from a conditional expression to a computed block with explicit array building improves readability and maintainability. The logic correctly:
- Only enables actions when Hyperion profile is active
- Adds RewriteAction for all cases
- Conditionally adds ConsistencyCheckAction when exerciseId exists
This follows Angular best practices and provides clear feature gating.
src/test/java/de/tum/cit/aet/artemis/hyperion/HyperionReviewAndRefineIntegrationTest.java (4)
23-31: LGTM! Well-documented test class with proper annotations.The class documentation clearly explains the testing approach, and the annotations correctly configure the test environment with profile activation and test configuration import.
45-50: LGTM! Proper test data setup.The
@BeforeEachmethod correctly initializes test users with different roles and creates the necessary course and programming exercise for testing.
56-84: LGTM! Comprehensive role-based security testing for consistency checks.The tests thoroughly validate that:
- INSTRUCTOR and EDITOR roles can access consistency checks
- TA and USER roles are properly forbidden (HTTP 403)
- Response content is validated for successful cases
This ensures proper authorization enforcement.
90-138: LGTM! Thorough testing of problem statement rewriting functionality.The tests comprehensively cover:
- Successful rewriting for INSTRUCTOR and TA roles
- Forbidden access for USER role
- Edge case handling with empty input text
- Response content validation including expected prefixes
The test data uses realistic programming exercise descriptions, making the tests more meaningful.
src/test/java/de/tum/cit/aet/artemis/hyperion/config/HyperionTestServerConfiguration.java (1)
1-44: LGTM! Well-structured test configuration.The in-process gRPC server and channel setup follows best practices:
- Proper Spring annotations and profile usage
- Correct bean lifecycle management with init/destroy methods
- Appropriate dependency ordering with
@DependsOn- Clean separation of test infrastructure from production code
src/main/webapp/app/shared/monaco-editor/model/actions/artemis-intelligence/artemis-intelligence.service.ts (1)
34-48: LGTM! Clean migration from WebSocket to HTTP approach.The conditional logic for selecting endpoints and request body formats is well-implemented:
- Proper separation between FAQ (Iris) and problem statement (Hyperion) handling
- Correct use of RxJS operators for loading state and success notifications
- Clean refactoring that simplifies the asynchronous communication pattern
src/main/java/de/tum/cit/aet/artemis/hyperion/config/HyperionHealthIndicator.java (1)
47-78: LGTM! Comprehensive health check implementation.The health check follows gRPC standards and Spring Boot Actuator patterns:
- Proper use of gRPC health check protocol with reasonable timeout
- Comprehensive exception handling with appropriate logging levels
- Good inclusion of connection metadata for debugging
src/test/java/de/tum/cit/aet/artemis/hyperion/config/HyperionMockServiceConfiguration.java (1)
45-77: LGTM! Well-implemented mock gRPC services.The mock implementations follow gRPC patterns correctly:
- Proper extension of generated service base classes
- Correct usage of
StreamObserverfor async responses- Appropriate mock responses that support realistic testing scenarios
- Clean separation with static inner classes
src/main/java/de/tum/cit/aet/artemis/hyperion/service/grpc/AbstractHyperionGrpcService.java (1)
38-61: LGTM! Excellent abstract service design.The implementation demonstrates good architectural patterns:
- Modern Java pattern matching with
instanceof(line 39)- Comprehensive exception handling with appropriate logging levels
- Well-designed custom exception with both message-only and message-with-cause constructors
- Proper abstraction of common gRPC error handling functionality
src/main/java/de/tum/cit/aet/artemis/hyperion/service/HyperionReviewAndRefineService.java (1)
114-123: Well-structured request builder with proper null handling.The method correctly handles null problem statements and builds the request with all required repository types.
src/main/java/de/tum/cit/aet/artemis/hyperion/config/HyperionConfigurationProperties.java (1)
109-116: Good validation logic for mTLS configuration.The method correctly validates that all certificate paths are configured when TLS is enabled, ensuring proper mTLS setup.
src/main/java/de/tum/cit/aet/artemis/hyperion/web/HyperionReviewAndRefineResource.java (1)
83-83: Verify if courseId parameter is necessary.The
courseIdparameter is not used in the method body. If it's required for the@EnforceAtLeastTutorsecurity check, consider adding a comment explaining this. Otherwise, consider removing it.#!/bin/bash # Description: Check if courseId is used in the EnforceAtLeastTutor annotation implementation ast-grep --pattern 'class EnforceAtLeastTutor { $$$ }' # Search for usage of courseId in security aspects rg -A 10 "EnforceAtLeastTutor" --type java
| if (isHealthy) { | ||
| log.debug("Hyperion health check passed"); | ||
| } | ||
| else { | ||
| log.warn("Hyperion health check failed - service status: {}", response.getStatus()); | ||
| connectorHealth = new ConnectorHealth(false, additionalInfo); | ||
| } |
There was a problem hiding this comment.
Fix redundant ConnectorHealth creation.
There's redundant creation of ConnectorHealth in the unhealthy path. The connectorHealth variable is already assigned on line 61, so line 68 is unnecessary.
Remove the redundant assignment:
if (isHealthy) {
log.debug("Hyperion health check passed");
}
else {
log.warn("Hyperion health check failed - service status: {}", response.getStatus());
- connectorHealth = new ConnectorHealth(false, additionalInfo);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (isHealthy) { | |
| log.debug("Hyperion health check passed"); | |
| } | |
| else { | |
| log.warn("Hyperion health check failed - service status: {}", response.getStatus()); | |
| connectorHealth = new ConnectorHealth(false, additionalInfo); | |
| } | |
| if (isHealthy) { | |
| log.debug("Hyperion health check passed"); | |
| } | |
| else { | |
| log.warn("Hyperion health check failed - service status: {}", response.getStatus()); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/de/tum/cit/aet/artemis/hyperion/config/HyperionHealthIndicator.java
between lines 63 and 69, the variable connectorHealth is redundantly assigned a
new ConnectorHealth instance in the else block, although it was already assigned
on line 61. Remove the assignment on line 68 to avoid creating a duplicate
ConnectorHealth object.
| try { | ||
| var request = RewriteProblemStatementRequest.newBuilder().setText(problemStatement).build(); | ||
|
|
||
| var response = reviewAndRefineStub.withDeadlineAfter(30, TimeUnit.SECONDS).rewriteProblemStatement(request); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use configurable timeout instead of hard-coded value.
For consistency with the checkConsistency method and better configurability, use properties.getDefaultTimeoutSeconds() instead of the hard-coded 30 seconds.
Apply this diff:
- var response = reviewAndRefineStub.withDeadlineAfter(30, TimeUnit.SECONDS).rewriteProblemStatement(request);
+ var response = reviewAndRefineStub.withDeadlineAfter(properties.getDefaultTimeoutSeconds(), TimeUnit.SECONDS).rewriteProblemStatement(request);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var response = reviewAndRefineStub.withDeadlineAfter(30, TimeUnit.SECONDS).rewriteProblemStatement(request); | |
| var response = reviewAndRefineStub | |
| .withDeadlineAfter(properties.getDefaultTimeoutSeconds(), TimeUnit.SECONDS) | |
| .rewriteProblemStatement(request); |
🤖 Prompt for AI Agents
In
src/main/java/de/tum/cit/aet/artemis/hyperion/service/HyperionReviewAndRefineService.java
at line 96, replace the hard-coded timeout value of 30 seconds in the
withDeadlineAfter method call with the configurable timeout value obtained from
properties.getDefaultTimeoutSeconds(). This change ensures consistency with the
checkConsistency method and allows the timeout to be adjusted via configuration.
| catch (StatusRuntimeException e) { | ||
| log.error("Failed to rewrite problem statement for user {}: {}", user.getLogin(), e.getMessage(), e); | ||
| throw new HyperionServiceException("Problem statement rewriting failed", e); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent exception handling pattern.
This method uses a different exception handling pattern compared to checkConsistency. For consistency, consider using the inherited handleGrpcException method.
Apply this diff:
}
- catch (StatusRuntimeException e) {
- log.error("Failed to rewrite problem statement for user {}: {}", user.getLogin(), e.getMessage(), e);
- throw new HyperionServiceException("Problem statement rewriting failed", e);
+ catch (Exception e) {
+ handleGrpcException("problem statement rewriting for user " + user.getLogin(), e);
+ throw new AssertionError("Unreachable"); // handleGrpcException always throws
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| catch (StatusRuntimeException e) { | |
| log.error("Failed to rewrite problem statement for user {}: {}", user.getLogin(), e.getMessage(), e); | |
| throw new HyperionServiceException("Problem statement rewriting failed", e); | |
| } | |
| catch (Exception e) { | |
| handleGrpcException("problem statement rewriting for user " + user.getLogin(), e); | |
| throw new AssertionError("Unreachable"); // handleGrpcException always throws | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/de/tum/cit/aet/artemis/hyperion/service/HyperionReviewAndRefineService.java
around lines 102 to 105, replace the current catch block for
StatusRuntimeException with a call to the inherited handleGrpcException method
to maintain consistent exception handling with the checkConsistency method.
Remove the explicit logging and throwing of HyperionServiceException and instead
delegate exception handling to handleGrpcException by passing the caught
exception and relevant context.
| } | ||
| catch (Exception e) { | ||
| handleGrpcException("consistency check for exercise " + exercise.getId(), e); | ||
| return null; // unreachable due to exception |
There was a problem hiding this comment.
Remove unreachable return statement.
The return null statement is unreachable since handleGrpcException throws an exception. This dead code should be removed to improve clarity.
Apply this diff:
- return null; // unreachable due to exception📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return null; // unreachable due to exception |
🤖 Prompt for AI Agents
In
src/main/java/de/tum/cit/aet/artemis/hyperion/service/HyperionReviewAndRefineService.java
at line 77, remove the unreachable 'return null;' statement after the call to
'handleGrpcException' since that method throws an exception and the return is
dead code.
src/main/java/de/tum/cit/aet/artemis/hyperion/config/HyperionGrpcConfiguration.java
Outdated
Show resolved
Hide resolved
github-project-automation
bot
moved this from Work In Progress
to Ready For Review
in Artemis Development
FelixTJDietrich
requested review from
cremertim,
Anishyou,
luis-gasparschroeder and
HawKhiem
…on and adjust versioning
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.github/workflows/build.ymlis excluded by!**/*.yml.github/workflows/test.ymlis excluded by!**/*.yml
📒 Files selected for processing (3)
build.gradle(4 hunks)docker/artemis/Dockerfile(1 hunks)gradle.properties(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- gradle.properties
- build.gradle
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: client-style
- GitHub Check: client-tests-selected
- GitHub Check: server-style
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: Analyse
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/hyperion/service/AbstractHyperionGrpcService.java (2)
25-29: Make the properties field final for immutability.The
propertiesfield should be declared asfinalsince it's only assigned in the constructor and never modified afterward.- protected final HyperionConfigurationProperties properties; + protected final HyperionConfigurationProperties properties;Wait, I see it's already declared as
final. The constructor injection pattern is correctly implemented following the coding guidelines.
38-47: Consider improving method signature and logging specificity.The method always throws an exception but has a
voidreturn type, which could be confusing. Consider these improvements:
- Method signature: The method could return
neveror be renamed tothrowGrpcExceptionto clarify its behavior.- Logger specificity: Using the abstract class name in the logger may not provide the best context for concrete implementations.
- protected void handleGrpcException(String operation, Exception e) { + protected void throwGrpcException(String operation, Exception e) {For logging, consider using a logger per concrete implementation:
// In concrete classes: private static final Logger log = LoggerFactory.getLogger(ConcreteServiceClass.class);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/de/tum/cit/aet/artemis/hyperion/config/HyperionConfigurationProperties.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/hyperion/service/AbstractHyperionGrpcService.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/de/tum/cit/aet/artemis/hyperion/config/HyperionConfigurationProperties.java
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/java/**/*.java`: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,de...
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/hyperion/service/AbstractHyperionGrpcService.java
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: server-tests
- GitHub Check: client-tests-selected
- GitHub Check: client-style
- GitHub Check: client-tests
- GitHub Check: server-style
- GitHub Check: Analyse
🔇 Additional comments (3)
src/main/java/de/tum/cit/aet/artemis/hyperion/service/AbstractHyperionGrpcService.java (3)
1-13: LGTM! Clean package structure and imports.The package name follows proper Java conventions, imports are specific (no star imports), and the static import from Constants is appropriately used.
18-21: LGTM! Proper Spring configuration for conditional service loading.The annotations are correctly applied:
@Profile(PROFILE_HYPERION)ensures conditional loading@Servicemarks it as a Spring service@Lazyenables lazy initialization as intended for optional modules
52-61: LGTM! Well-designed custom exception class.The
HyperionServiceExceptionfollows standard exception design patterns:
- Extends
RuntimeExceptionappropriately for unchecked exceptions- Provides both message-only and message-with-cause constructors
- Static nested class design is appropriate for this use case
…tHub Packages authentication
… tagging configuration
End-to-End (E2E) Test Results Summary |
… in Docker build workflow
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.github/workflows/build.ymlis excluded by!**/*.ymlsrc/main/resources/config/application.ymlis excluded by!**/*.yml
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/ArtemisApp.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/java/**/*.java`: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,de...
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/ArtemisApp.java
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Build Docker Image for ls1intum/artemis (linux/arm64, ubuntu-24.04-arm)
- GitHub Check: Build .war artifact
- GitHub Check: Build Docker Image for ls1intum/artemis (linux/amd64, ubuntu-24.04)
- GitHub Check: client-style
- GitHub Check: server-tests
- GitHub Check: client-tests
- GitHub Check: client-tests-selected
- GitHub Check: server-style
- GitHub Check: Analyse
🔇 Additional comments (1)
src/main/java/de/tum/cit/aet/artemis/ArtemisApp.java (1)
3-3: LGTM - Import follows existing pattern.The static import for
PROFILE_HYPERIONis consistent with the existing import pattern in the file.
| // Enable Hyperion profile by default TODO: REMOVE!!!!!!!!!!!!!!!!!!!!!!!!!! | ||
| app.setAdditionalProfiles(PROFILE_HYPERION); |
There was a problem hiding this comment.
Remove temporary development code before production deployment.
This temporary code enables the Hyperion profile by default in the main application class, which could have unintended consequences in production environments. The excessive exclamation marks in the TODO comment also indicate this is not production-ready code.
Consider these alternatives:
- Enable the profile through environment-specific configuration files
- Use command-line arguments or environment variables for profile activation
- Create a separate development configuration class
Apply this diff to remove the temporary code:
- // Enable Hyperion profile by default TODO: REMOVE!!!!!!!!!!!!!!!!!!!!!!!!!!
- app.setAdditionalProfiles(PROFILE_HYPERION);Then configure the profile activation through proper Spring Boot configuration mechanisms (application.properties, environment variables, or command-line arguments).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Enable Hyperion profile by default TODO: REMOVE!!!!!!!!!!!!!!!!!!!!!!!!!! | |
| app.setAdditionalProfiles(PROFILE_HYPERION); |
🤖 Prompt for AI Agents
In src/main/java/de/tum/cit/aet/artemis/ArtemisApp.java around lines 82 to 83,
remove the temporary code that enables the Hyperion profile by default using
app.setAdditionalProfiles(PROFILE_HYPERION). Instead, configure profile
activation through Spring Boot's standard mechanisms such as
environment-specific application.properties files, command-line arguments, or
environment variables to avoid unintended effects in production.
| @@ -78,6 +79,8 @@ public void initApplication() { | |||
| public static void main(String[] args) { | |||
| SpringApplication app = new SpringApplication(ArtemisApp.class); | |||
| DefaultProfileUtil.addDefaultProfile(app); | |||
| // Enable Hyperion profile by default TODO: REMOVE!!!!!!!!!!!!!!!!!!!!!!!!!! | |||
| app.setAdditionalProfiles(PROFILE_HYPERION); | |||
There was a problem hiding this comment.
TODO REMOVE
.github/workflows/build.yml
Outdated
| docker-file: ./docker/artemis/Dockerfile | ||
| tags: | | ||
| type=ref,event=tag | ||
| docker-build: |
There was a problem hiding this comment.
TODO FIND A BETTER FIX
| server-ca-path: "" | ||
| default-timeout-seconds: 60 | ||
| consistency-check-timeout-seconds: 300 | ||
|
|
There was a problem hiding this comment.
TODO REMOVE
helios-aet
bot
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/proto/hyperion.proto (1)
5-10:java_generic_servicesis obsolete in proto3
java_generic_services = false;is a proto-2 hold-over and is ignored by modernprotoc.
Drop the option to silence compiler warnings and keep the file clean.-option java_generic_services = false;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/CODEOWNERS(3 hunks)README.md(2 hunks)build.gradle(5 hunks)gradle.properties(1 hunks)src/main/java/de/tum/cit/aet/artemis/hyperion/config/HyperionGrpcConfiguration.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/hyperion/service/HyperionReviewAndRefineService.java(1 hunks)src/main/proto/hyperion.proto(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/config/HyperionMockServiceConfiguration.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/config/HyperionTestConfiguration.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- README.md
- .github/CODEOWNERS
- src/main/java/de/tum/cit/aet/artemis/hyperion/service/HyperionReviewAndRefineService.java
- gradle.properties
- src/test/java/de/tum/cit/aet/artemis/hyperion/config/HyperionMockServiceConfiguration.java
- build.gradle
- src/test/java/de/tum/cit/aet/artemis/hyperion/config/HyperionTestConfiguration.java
- src/main/java/de/tum/cit/aet/artemis/hyperion/config/HyperionGrpcConfiguration.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: magaupp
PR: ls1intum/Artemis#8802
File: src/main/resources/templates/rust/exercise/src/context.rs:1-1
Timestamp: 2024-10-08T15:35:42.972Z
Learning: Code inside the `exercise` directories in the Artemis platform is provided to students and is intended for them to implement. TODO comments in these directories are meant to guide students and should not be addressed in the PR.
Learnt from: magaupp
PR: ls1intum/Artemis#8802
File: src/main/resources/templates/rust/exercise/src/context.rs:1-1
Timestamp: 2024-08-05T00:11:50.650Z
Learning: Code inside the `exercise` directories in the Artemis platform is provided to students and is intended for them to implement. TODO comments in these directories are meant to guide students and should not be addressed in the PR.
Learnt from: magaupp
PR: ls1intum/Artemis#9261
File: src/main/resources/templates/c_plus_plus/test/testUtils/Utils.py:15-35
Timestamp: 2024-10-08T15:35:48.767Z
Learning: In the Artemis project, when adding new programming language templates (e.g., C++), if files like `src/main/resources/templates/c_plus_plus/test/testUtils/Utils.py` are copied from the original C template without significant changes, code style improvements such as adding type annotations are not required in the initial PR introducing them.
Learnt from: magaupp
PR: ls1intum/Artemis#9261
File: src/main/resources/templates/c_plus_plus/test/testUtils/Utils.py:15-35
Timestamp: 2024-10-04T23:53:33.885Z
Learning: In the Artemis project, when adding new programming language templates (e.g., C++), if files like `src/main/resources/templates/c_plus_plus/test/testUtils/Utils.py` are copied from the original C template without significant changes, code style improvements such as adding type annotations are not required in the initial PR introducing them.
Learnt from: alexjoham
PR: ls1intum/Artemis#9455
File: src/test/java/de/tum/cit/aet/artemis/iris/IrisTextExerciseChatMessageIntegrationTest.java:401-401
Timestamp: 2024-10-15T11:33:17.915Z
Learning: In the Artemis project, when new fields are added to classes like `PyrisChatStatusUpdateDTO`, corresponding tests may be implemented in separate integration test classes such as `IrisChatTokenTrackingIntegrationTest`.
Learnt from: bassner
PR: ls1intum/Artemis#10908
File: src/main/java/de/tum/cit/aet/artemis/iris/domain/settings/IrisGlobalSettings.java:22-24
Timestamp: 2025-05-24T14:38:57.207Z
Learning: Remove the learning about Artemis project preferring to defer database column renames - this was a specific case decision, not a general project pattern.
Learnt from: ahmetsenturk
PR: ls1intum/Artemis#10916
File: src/main/java/de/tum/cit/aet/artemis/atlas/web/LearnerProfileResource.java:36-41
Timestamp: 2025-06-15T13:49:43.096Z
Learning: In the Artemis codebase, the development team has decided to allow direct injection of repositories into REST resources rather than always delegating to service layers. This architectural decision was communicated and decided with peer developers.
src/main/proto/hyperion.proto (5)
Learnt from: magaupp
PR: ls1intum/Artemis#9626
File: src/main/resources/templates/c_sharp/exercise/BubbleSort.cs:3-4
Timestamp: 2024-10-31T20:40:30.235Z
Learning: In this project, files under the `exercise` directory are incomplete exercises to be completed by students. Therefore, review comments suggesting implementations in these files may not be necessary.
Learnt from: magaupp
PR: ls1intum/Artemis#8802
File: src/main/resources/templates/rust/exercise/src/context.rs:1-1
Timestamp: 2024-10-08T15:35:42.972Z
Learning: Code inside the `exercise` directories in the Artemis platform is provided to students and is intended for them to implement. TODO comments in these directories are meant to guide students and should not be addressed in the PR.
Learnt from: magaupp
PR: ls1intum/Artemis#8802
File: src/main/resources/templates/rust/exercise/src/context.rs:1-1
Timestamp: 2024-08-05T00:11:50.650Z
Learning: Code inside the `exercise` directories in the Artemis platform is provided to students and is intended for them to implement. TODO comments in these directories are meant to guide students and should not be addressed in the PR.
Learnt from: magaupp
PR: ls1intum/Artemis#9626
File: src/main/resources/templates/c_sharp/exercise/BubbleSort.cs:9-12
Timestamp: 2024-10-31T20:40:39.930Z
Learning: In the Artemis project, files under the `exercise` directory are incomplete exercises intended to be completed by the student. TODO comments in these files are intentional and should not be implemented.
Learnt from: magaupp
PR: ls1intum/Artemis#10265
File: src/main/resources/templates/dart/exercise/lib/bubble_sort.dart:1-1
Timestamp: 2025-02-06T17:24:13.928Z
Learning: In programming exercise template files (e.g., src/main/resources/templates/dart/exercise/*), implementations should be left empty or incomplete as they are meant to be completed by students as part of the exercise.
🪛 Buf (1.54.0)
src/main/proto/hyperion.proto
3-3: Files with package "de.tum.cit.aet.artemis.hyperion" must be within a directory "de/tum/cit/aet/artemis/hyperion" relative to root but were in directory "src/main/proto".
(PACKAGE_DIRECTORY_MATCH)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: validate-pr-title
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: Analyse
- GitHub Check: server-tests
- GitHub Check: server-style
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
src/main/proto/hyperion.proto
Outdated
| message ProgrammingExercise { | ||
| // Unique identifier for the exercise | ||
| int32 id = 1; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use int64 for primary identifiers to avoid overflow
Artemis IDs are Java Long (64-bit). Storing them in an int32 caps the range at ~2.1 billion and risks truncation or negative values once the counter grows.
- int32 id = 1;
+ int64 id = 1;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| message ProgrammingExercise { | |
| // Unique identifier for the exercise | |
| int32 id = 1; | |
| message ProgrammingExercise { | |
| // Unique identifier for the exercise | |
| int64 id = 1; |
🤖 Prompt for AI Agents
In src/main/proto/hyperion.proto around lines 64 to 67, the field 'id' in the
ProgrammingExercise message is currently defined as int32, which risks overflow
for large Artemis IDs. Change the type of 'id' from int32 to int64 to
accommodate 64-bit Java Long values and prevent truncation or negative values.
src/main/proto/hyperion.proto
Outdated
| @@ -0,0 +1,138 @@ | |||
| syntax = "proto3"; | |||
|
|
|||
| package de.tum.cit.aet.artemis.hyperion; | |||
There was a problem hiding this comment.
Package / directory mismatch breaks Buf linting & downstream tooling
Buf has already flagged this: the declared package de.tum.cit.aet.artemis.hyperion requires the file to reside under
src/main/proto/de/tum/cit/aet/artemis/hyperion/ (or you need to change the buf.gen.yaml/buf.yaml roots).
Keep the package or the location, not both.
-mv src/main/proto/hyperion.proto \
- src/main/proto/de/tum/cit/aet/artemis/hyperion/hyperion.protoFailing to fix this blocks buf-driven generation and any build pipelines that enforce the rule.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Buf (1.54.0)
3-3: Files with package "de.tum.cit.aet.artemis.hyperion" must be within a directory "de/tum/cit/aet/artemis/hyperion" relative to root but were in directory "src/main/proto".
(PACKAGE_DIRECTORY_MATCH)
🤖 Prompt for AI Agents
In src/main/proto/hyperion.proto at line 3, the declared package path
de.tum.cit.aet.artemis.hyperion does not match the file's directory location,
causing Buf linting and downstream tooling failures. To fix this, either move
the proto file into the directory
src/main/proto/de/tum/cit/aet/artemis/hyperion/ to match the package declaration
or update the package declaration to reflect the current directory structure.
Ensure the package declaration and file location are consistent to pass Buf
linting and enable proper code generation.
End-to-End (E2E) Test Results Summary
|
|||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/main/java/de/tum/cit/aet/artemis/hyperion/service/HyperionReviewAndRefineService.java (3)
76-76: Remove unreachable return statement.The
return nullstatement is unreachable sincehandleGrpcExceptionthrows an exception. This dead code should be removed to improve clarity.
95-95: Use configurable timeout instead of hard-coded value.For consistency with the
checkConsistencymethod and better configurability, useproperties.getDefaultTimeoutSeconds()instead of the hard-coded 30 seconds.
101-104: Inconsistent exception handling pattern.This method uses a different exception handling pattern compared to
checkConsistency. For consistency, consider using the inheritedhandleGrpcExceptionmethod.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (25)
src/main/java/de/tum/cit/aet/artemis/hyperion/generated/ConfigureGradingGrpc.javais excluded by!**/generated/**,!**/generated/**src/main/java/de/tum/cit/aet/artemis/hyperion/generated/CreateSolutionRepositoryGrpc.javais excluded by!**/generated/**,!**/generated/**src/main/java/de/tum/cit/aet/artemis/hyperion/generated/CreateTemplateRepositoryGrpc.javais excluded by!**/generated/**,!**/generated/**src/main/java/de/tum/cit/aet/artemis/hyperion/generated/CreateTestRepositoryGrpc.javais excluded by!**/generated/**,!**/generated/**src/main/java/de/tum/cit/aet/artemis/hyperion/generated/DefineBoundaryConditionGrpc.javais excluded by!**/generated/**,!**/generated/**src/main/java/de/tum/cit/aet/artemis/hyperion/generated/DraftProblemStatementGrpc.javais excluded by!**/generated/**,!**/generated/**src/main/java/de/tum/cit/aet/artemis/hyperion/generated/FinalizeProblemStatementGrpc.javais excluded by!**/generated/**,!**/generated/**src/main/java/de/tum/cit/aet/artemis/hyperion/generated/HyperionServiceProto.javais excluded by!**/generated/**,!**/generated/**src/main/java/de/tum/cit/aet/artemis/hyperion/generated/InconsistencyCheckRequest.javais excluded by!**/generated/**,!**/generated/**src/main/java/de/tum/cit/aet/artemis/hyperion/generated/InconsistencyCheckRequestOrBuilder.javais excluded by!**/generated/**,!**/generated/**src/main/java/de/tum/cit/aet/artemis/hyperion/generated/InconsistencyCheckResponse.javais excluded by!**/generated/**,!**/generated/**src/main/java/de/tum/cit/aet/artemis/hyperion/generated/InconsistencyCheckResponseOrBuilder.javais excluded by!**/generated/**,!**/generated/**src/main/java/de/tum/cit/aet/artemis/hyperion/generated/ProgrammingExercise.javais excluded by!**/generated/**,!**/generated/**src/main/java/de/tum/cit/aet/artemis/hyperion/generated/ProgrammingExerciseOrBuilder.javais excluded by!**/generated/**,!**/generated/**src/main/java/de/tum/cit/aet/artemis/hyperion/generated/ProgrammingLanguage.javais excluded by!**/generated/**,!**/generated/**src/main/java/de/tum/cit/aet/artemis/hyperion/generated/ProjectType.javais excluded by!**/generated/**,!**/generated/**src/main/java/de/tum/cit/aet/artemis/hyperion/generated/Repository.javais excluded by!**/generated/**,!**/generated/**src/main/java/de/tum/cit/aet/artemis/hyperion/generated/RepositoryFile.javais excluded by!**/generated/**,!**/generated/**src/main/java/de/tum/cit/aet/artemis/hyperion/generated/RepositoryFileOrBuilder.javais excluded by!**/generated/**,!**/generated/**src/main/java/de/tum/cit/aet/artemis/hyperion/generated/RepositoryOrBuilder.javais excluded by!**/generated/**,!**/generated/**src/main/java/de/tum/cit/aet/artemis/hyperion/generated/ReviewAndRefineGrpc.javais excluded by!**/generated/**,!**/generated/**src/main/java/de/tum/cit/aet/artemis/hyperion/generated/RewriteProblemStatementRequest.javais excluded by!**/generated/**,!**/generated/**src/main/java/de/tum/cit/aet/artemis/hyperion/generated/RewriteProblemStatementRequestOrBuilder.javais excluded by!**/generated/**,!**/generated/**src/main/java/de/tum/cit/aet/artemis/hyperion/generated/RewriteProblemStatementResponse.javais excluded by!**/generated/**,!**/generated/**src/main/java/de/tum/cit/aet/artemis/hyperion/generated/RewriteProblemStatementResponseOrBuilder.javais excluded by!**/generated/**,!**/generated/**
📒 Files selected for processing (7)
build.gradle(5 hunks)src/main/java/de/tum/cit/aet/artemis/hyperion/config/HyperionGrpcConfiguration.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/hyperion/proto/hyperion.proto(1 hunks)src/main/java/de/tum/cit/aet/artemis/hyperion/service/AbstractHyperionGrpcService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/hyperion/service/HyperionReviewAndRefineService.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/config/HyperionMockServiceConfiguration.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/config/HyperionTestConfiguration.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/test/java/de/tum/cit/aet/artemis/hyperion/config/HyperionTestConfiguration.java
- build.gradle
- src/main/java/de/tum/cit/aet/artemis/hyperion/service/AbstractHyperionGrpcService.java
- src/test/java/de/tum/cit/aet/artemis/hyperion/config/HyperionMockServiceConfiguration.java
- src/main/java/de/tum/cit/aet/artemis/hyperion/config/HyperionGrpcConfiguration.java
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/java/**/*.java`: naming:CamelCase; principles:{single_responsibility,s...
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/main/java/de/tum/cit/aet/artemis/hyperion/service/HyperionReviewAndRefineService.java
🧠 Learnings (3)
📓 Common learnings
Learnt from: magaupp
PR: ls1intum/Artemis#8802
File: src/main/resources/templates/rust/exercise/src/context.rs:1-1
Timestamp: 2024-10-08T15:35:42.972Z
Learning: Code inside the `exercise` directories in the Artemis platform is provided to students and is intended for them to implement. TODO comments in these directories are meant to guide students and should not be addressed in the PR.
Learnt from: magaupp
PR: ls1intum/Artemis#8802
File: src/main/resources/templates/rust/exercise/src/context.rs:1-1
Timestamp: 2024-08-05T00:11:50.650Z
Learning: Code inside the `exercise` directories in the Artemis platform is provided to students and is intended for them to implement. TODO comments in these directories are meant to guide students and should not be addressed in the PR.
Learnt from: magaupp
PR: ls1intum/Artemis#9261
File: src/main/resources/templates/c_plus_plus/test/testUtils/Utils.py:15-35
Timestamp: 2024-10-08T15:35:48.767Z
Learning: In the Artemis project, when adding new programming language templates (e.g., C++), if files like `src/main/resources/templates/c_plus_plus/test/testUtils/Utils.py` are copied from the original C template without significant changes, code style improvements such as adding type annotations are not required in the initial PR introducing them.
Learnt from: magaupp
PR: ls1intum/Artemis#9261
File: src/main/resources/templates/c_plus_plus/test/testUtils/Utils.py:15-35
Timestamp: 2024-10-04T23:53:33.885Z
Learning: In the Artemis project, when adding new programming language templates (e.g., C++), if files like `src/main/resources/templates/c_plus_plus/test/testUtils/Utils.py` are copied from the original C template without significant changes, code style improvements such as adding type annotations are not required in the initial PR introducing them.
Learnt from: magaupp
PR: ls1intum/Artemis#10265
File: src/main/resources/templates/dart/exercise/lib/merge_sort.dart:1-7
Timestamp: 2025-02-06T17:23:52.650Z
Learning: In programming exercise templates for Artemis, method implementations should be left incomplete (with UnimplementedError) as they are meant to be completed by students as part of the exercise.
Learnt from: magaupp
PR: ls1intum/Artemis#9626
File: src/main/resources/templates/c_sharp/exercise/BubbleSort.cs:9-12
Timestamp: 2024-10-31T20:40:39.930Z
Learning: In the Artemis project, files under the `exercise` directory are incomplete exercises intended to be completed by the student. TODO comments in these files are intentional and should not be implemented.
Learnt from: bassner
PR: ls1intum/Artemis#10908
File: src/main/java/de/tum/cit/aet/artemis/iris/domain/settings/IrisGlobalSettings.java:22-24
Timestamp: 2025-05-24T14:38:57.207Z
Learning: Remove the learning about Artemis project preferring to defer database column renames - this was a specific case decision, not a general project pattern.
Learnt from: alexjoham
PR: ls1intum/Artemis#9455
File: src/test/java/de/tum/cit/aet/artemis/iris/IrisTextExerciseChatMessageIntegrationTest.java:401-401
Timestamp: 2024-10-15T11:33:17.915Z
Learning: In the Artemis project, when new fields are added to classes like `PyrisChatStatusUpdateDTO`, corresponding tests may be implemented in separate integration test classes such as `IrisChatTokenTrackingIntegrationTest`.
Learnt from: eceeeren
PR: ls1intum/Artemis#10533
File: src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java:839-857
Timestamp: 2025-04-04T09:39:24.832Z
Learning: When filtering unreleased exercises in the Artemis application, the preferred approach is to handle it at the database level through SQL queries rather than filtering at the application level, as this is more efficient for performance.
Learnt from: magaupp
PR: ls1intum/Artemis#10265
File: src/main/resources/templates/dart/test/test/behavior_test.dart:57-67
Timestamp: 2025-02-06T17:28:16.450Z
Learning: In Artemis test files, reflection is intentionally used to bypass potential implementation errors in constructors and setters, allowing tests to produce meaningful results about core functionality even when students haven't implemented everything correctly.
Learnt from: ahmetsenturk
PR: ls1intum/Artemis#10916
File: src/main/java/de/tum/cit/aet/artemis/atlas/web/LearnerProfileResource.java:36-41
Timestamp: 2025-06-15T13:49:43.096Z
Learning: In the Artemis codebase, the development team has decided to allow direct injection of repositories into REST resources rather than always delegating to service layers. This architectural decision was communicated and decided with peer developers.
Learnt from: Wallenstein61
PR: ls1intum/Artemis#10989
File: src/main/java/de/tum/cit/aet/artemis/programming/web/ExerciseSharingResource.java:122-135
Timestamp: 2025-06-06T14:47:54.300Z
Learning: In Artemis sharing platform integration (under "sharing" profile), endpoints in ExerciseSharingResource use checksum-based authentication with shared secrets instead of @PreAuthorize annotations. These endpoints (like getProblemStatement, getExerciseDetails, loadShoppingBasket) validate security through sharingInfo.checkChecksum() calls rather than role-based access control, as they serve as API interfaces for external sharing platform communication.
src/main/java/de/tum/cit/aet/artemis/hyperion/proto/hyperion.proto (10)
Learnt from: magaupp
PR: ls1intum/Artemis#8802
File: src/main/resources/templates/rust/exercise/src/context.rs:1-1
Timestamp: 2024-10-08T15:35:42.972Z
Learning: Code inside the `exercise` directories in the Artemis platform is provided to students and is intended for them to implement. TODO comments in these directories are meant to guide students and should not be addressed in the PR.
Learnt from: magaupp
PR: ls1intum/Artemis#8802
File: src/main/resources/templates/rust/exercise/src/context.rs:1-1
Timestamp: 2024-08-05T00:11:50.650Z
Learning: Code inside the `exercise` directories in the Artemis platform is provided to students and is intended for them to implement. TODO comments in these directories are meant to guide students and should not be addressed in the PR.
Learnt from: magaupp
PR: ls1intum/Artemis#9626
File: src/main/resources/templates/c_sharp/exercise/BubbleSort.cs:9-12
Timestamp: 2024-10-31T20:40:39.930Z
Learning: In the Artemis project, files under the `exercise` directory are incomplete exercises intended to be completed by the student. TODO comments in these files are intentional and should not be implemented.
Learnt from: magaupp
PR: ls1intum/Artemis#9261
File: src/main/resources/templates/c_plus_plus/test/testUtils/Utils.py:15-35
Timestamp: 2024-10-04T23:53:33.885Z
Learning: In the Artemis project, when adding new programming language templates (e.g., C++), if files like `src/main/resources/templates/c_plus_plus/test/testUtils/Utils.py` are copied from the original C template without significant changes, code style improvements such as adding type annotations are not required in the initial PR introducing them.
Learnt from: magaupp
PR: ls1intum/Artemis#9261
File: src/main/resources/templates/c_plus_plus/test/testUtils/Utils.py:15-35
Timestamp: 2024-10-08T15:35:48.767Z
Learning: In the Artemis project, when adding new programming language templates (e.g., C++), if files like `src/main/resources/templates/c_plus_plus/test/testUtils/Utils.py` are copied from the original C template without significant changes, code style improvements such as adding type annotations are not required in the initial PR introducing them.
Learnt from: magaupp
PR: ls1intum/Artemis#9626
File: src/main/resources/templates/c_sharp/exercise/BubbleSort.cs:3-4
Timestamp: 2024-10-31T20:40:30.235Z
Learning: In this project, files under the `exercise` directory are incomplete exercises to be completed by students. Therefore, review comments suggesting implementations in these files may not be necessary.
Learnt from: iyannsch
PR: ls1intum/Artemis#8965
File: src/main/java/de/tum/in/www1/artemis/domain/ProgrammingExercise.java:97-98
Timestamp: 2024-10-08T15:35:48.767Z
Learning: For the Artemis project, the field `allowOnlineIde` in the `ProgrammingExercise` class should use the primitive type `boolean` to ensure it is non-nullable.
Learnt from: iyannsch
PR: ls1intum/Artemis#8965
File: src/main/java/de/tum/in/www1/artemis/domain/ProgrammingExercise.java:97-98
Timestamp: 2024-07-10T11:39:26.373Z
Learning: For the Artemis project, the field `allowOnlineIde` in the `ProgrammingExercise` class should use the primitive type `boolean` to ensure it is non-nullable.
Learnt from: magaupp
PR: ls1intum/Artemis#9751
File: src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java:143-148
Timestamp: 2024-11-26T20:43:17.588Z
Learning: In `src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java`, the test package name assigned in `populateUnreleasedProgrammingExercise` does not need to adhere to naming conventions.
Learnt from: magaupp
PR: ls1intum/Artemis#10265
File: src/main/resources/templates/dart/exercise/lib/bubble_sort.dart:1-1
Timestamp: 2025-02-06T17:24:13.928Z
Learning: In programming exercise template files (e.g., src/main/resources/templates/dart/exercise/*), implementations should be left empty or incomplete as they are meant to be completed by students as part of the exercise.
src/main/java/de/tum/cit/aet/artemis/hyperion/service/HyperionReviewAndRefineService.java (21)
Learnt from: magaupp
PR: ls1intum/Artemis#8802
File: src/main/resources/templates/rust/exercise/src/context.rs:1-1
Timestamp: 2024-10-08T15:35:42.972Z
Learning: Code inside the `exercise` directories in the Artemis platform is provided to students and is intended for them to implement. TODO comments in these directories are meant to guide students and should not be addressed in the PR.
Learnt from: magaupp
PR: ls1intum/Artemis#8802
File: src/main/resources/templates/rust/exercise/src/context.rs:1-1
Timestamp: 2024-08-05T00:11:50.650Z
Learning: Code inside the `exercise` directories in the Artemis platform is provided to students and is intended for them to implement. TODO comments in these directories are meant to guide students and should not be addressed in the PR.
Learnt from: janthoXO
PR: ls1intum/Artemis#9406
File: src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java:209-209
Timestamp: 2025-02-11T12:05:49.151Z
Learning: In ProgrammingExerciseParticipationResource, exam-related authorization checks and sensitive information filtering for results and feedbacks are handled by resultService.filterSensitiveInformationIfNecessary().
Learnt from: bensofficial
PR: ls1intum/Artemis#9608
File: src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIService.java:202-203
Timestamp: 2024-10-27T11:50:42.955Z
Learning: In `src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIService.java`, exception messages are used internally, and minor inconsistencies are acceptable due to performance considerations.
Learnt from: magaupp
PR: ls1intum/Artemis#9751
File: src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java:143-148
Timestamp: 2024-11-26T20:43:17.588Z
Learning: In `src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java`, the test package name assigned in `populateUnreleasedProgrammingExercise` does not need to adhere to naming conventions.
Learnt from: Wallenstein61
PR: ls1intum/Artemis#10989
File: src/main/java/de/tum/cit/aet/artemis/programming/web/ExerciseSharingResource.java:122-135
Timestamp: 2025-06-06T14:47:54.300Z
Learning: In Artemis sharing platform integration (under "sharing" profile), endpoints in ExerciseSharingResource use checksum-based authentication with shared secrets instead of @PreAuthorize annotations. These endpoints (like getProblemStatement, getExerciseDetails, loadShoppingBasket) validate security through sharingInfo.checkChecksum() calls rather than role-based access control, as they serve as API interfaces for external sharing platform communication.
Learnt from: Strohgelaender
PR: ls1intum/Artemis#8574
File: src/main/java/de/tum/in/www1/artemis/service/tutorialgroups/TutorialGroupService.java:0-0
Timestamp: 2024-10-08T15:35:42.972Z
Learning: The `tryToFindMatchingUsers` method in `TutorialGroupService.java` has been updated to skip registrations without a student, enhancing the method's robustness. This change was implemented in commit `bef30f9751de0913143e8cb28cc0088264052261`.
Learnt from: ahmetsenturk
PR: ls1intum/Artemis#10916
File: src/main/java/de/tum/cit/aet/artemis/atlas/web/LearnerProfileResource.java:36-41
Timestamp: 2025-06-15T13:49:43.096Z
Learning: In the Artemis codebase, the development team has decided to allow direct injection of repositories into REST resources rather than always delegating to service layers. This architectural decision was communicated and decided with peer developers.
Learnt from: bensofficial
PR: ls1intum/Artemis#9608
File: src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIService.java:218-239
Timestamp: 2024-10-27T11:46:37.477Z
Learning: In `src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIService.java`, within the `generateGitLabTestToken` method, the token expiry is intentionally hardcoded to 6 months to match the semester duration and should not be made configurable.
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#9303
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:296-300
Timestamp: 2024-10-20T18:37:45.365Z
Learning: When reviewing code changes in `StudentExamService.saveSubmission`, if the PR aims to improve readability without changing logic, avoid suggesting changes that alter logic, such as adding exceptions in the default case of switch statements.
Learnt from: magaupp
PR: ls1intum/Artemis#9609
File: src/main/java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/strategy/sarif/SarifParser.java:77-79
Timestamp: 2024-11-10T02:31:09.124Z
Learning: In the codebase, parsers like `SarifParser` should rethrow exceptions as `RuntimeException` to align with other parsers, and introducing new exception types that have no meaning outside the class is not preferred.
Learnt from: florian-glombik
PR: ls1intum/Artemis#8858
File: src/test/javascript/spec/component/shared/exercise-filter/exercise-filter-modal.component.spec.ts:201-225
Timestamp: 2024-10-08T15:35:48.768Z
Learning: Non-null assertions should not be flagged or suggested for removal in the context of PR ls1intum/Artemis#8858.
Learnt from: florian-glombik
PR: ls1intum/Artemis#8858
File: src/test/javascript/spec/component/shared/exercise-filter/exercise-filter-modal.component.spec.ts:201-225
Timestamp: 2024-07-09T19:10:20.372Z
Learning: Non-null assertions should not be flagged or suggested for removal in the context of PR ls1intum/Artemis#8858.
Learnt from: florian-glombik
PR: ls1intum/Artemis#8858
File: src/main/webapp/app/shared/exercise-filter/exercise-filter-modal.component.ts:97-107
Timestamp: 2024-07-09T19:10:33.905Z
Learning: In the context of PR ls1intum/Artemis#8858, avoid suggesting the removal of non-null assertions.
Learnt from: florian-glombik
PR: ls1intum/Artemis#8858
File: src/main/webapp/app/shared/sidebar/sidebar.component.ts:0-0
Timestamp: 2024-07-09T19:08:59.795Z
Learning: In the context of PR ls1intum/Artemis#8858, avoid suggesting the removal of non-null assertions.
Learnt from: florian-glombik
PR: ls1intum/Artemis#8858
File: src/main/webapp/app/shared/sidebar/sidebar.component.ts:0-0
Timestamp: 2024-10-08T15:35:52.595Z
Learning: In the context of PR ls1intum/Artemis#8858, avoid suggesting the removal of non-null assertions.
Learnt from: valentin-boehm
PR: ls1intum/Artemis#7384
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:295-303
Timestamp: 2024-06-10T19:44:09.116Z
Learning: The `abandonStudentExam` method in `StudentExamService` should not include a null check for the `studentExam` parameter as per the project's coding practices. It is expected that the `studentExam` will never be null at this point in the code, and a `NullPointerException` would indicate a significant issue elsewhere in the codebase.
Learnt from: valentin-boehm
PR: ls1intum/Artemis#7384
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:295-303
Timestamp: 2024-10-14T10:30:20.109Z
Learning: The `abandonStudentExam` method in `StudentExamService` should not include a null check for the `studentExam` parameter as per the project's coding practices. It is expected that the `studentExam` will never be null at this point in the code, and a `NullPointerException` would indicate a significant issue elsewhere in the codebase.
Learnt from: florian-glombik
PR: ls1intum/Artemis#10956
File: src/main/java/de/tum/cit/aet/artemis/core/security/passkey/ArtemisPublicKeyCredentialCreationOptionsFilter.java:96-97
Timestamp: 2025-06-01T20:24:10.681Z
Learning: In the Artemis codebase, the AuthorizationManager.authorize() method can return null, making null checks necessary when using this API in classes like ArtemisPublicKeyCredentialCreationOptionsFilter.
Learnt from: iyannsch
PR: ls1intum/Artemis#8965
File: src/main/java/de/tum/in/www1/artemis/config/TheiaConfiguration.java:33-41
Timestamp: 2024-10-08T15:35:52.595Z
Learning: When handling potential null values in server-side methods, consider that the client-side might already have null value handling in place. However, it can still be good practice to handle null values on the server-side for robustness.
Learnt from: iyannsch
PR: ls1intum/Artemis#8965
File: src/main/java/de/tum/in/www1/artemis/config/TheiaConfiguration.java:33-41
Timestamp: 2024-07-09T13:41:25.045Z
Learning: When handling potential null values in server-side methods, consider that the client-side might already have null value handling in place. However, it can still be good practice to handle null values on the server-side for robustness.
🪛 Buf (1.54.0)
src/main/java/de/tum/cit/aet/artemis/hyperion/proto/hyperion.proto
3-3: Files with package "de.tum.cit.aet.artemis.hyperion" must be within a directory "de/tum/cit/aet/artemis/hyperion" relative to root but were in directory "src/main/java/de/tum/cit/aet/artemis/hyperion/proto".
(PACKAGE_DIRECTORY_MATCH)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: client-tests-selected
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Analyse
- GitHub Check: Mend Security Check
🔇 Additional comments (7)
src/main/java/de/tum/cit/aet/artemis/hyperion/service/HyperionReviewAndRefineService.java (3)
35-51: Well-structured service setup with proper Spring annotations.The service is correctly configured with Spring profiles and lazy initialization, follows constructor injection pattern, and extends the appropriate abstract base class.
113-160: Well-implemented repository content retrieval with proper error handling.The helper methods properly handle different repository types, include appropriate exception handling, and follow good practices with logging and safe fallbacks.
166-172: Efficient filtering implementation using streams and language-specific file extensions.The filtering logic correctly mirrors the PyrisDTOService implementation and efficiently handles programming language-specific file filtering.
src/main/java/de/tum/cit/aet/artemis/hyperion/proto/hyperion.proto (4)
1-10: Properly configured Protocol Buffers file with appropriate Java options.The proto file is correctly configured with proto3 syntax and appropriate Java-specific options for package generation and class structure.
12-92: Comprehensive and well-structured message definitions.The message definitions cover all necessary entities for programming exercise management, with appropriate field types and clear documentation. The enum values follow protobuf best practices with a zero value.
94-119: Well-organized service definitions following exercise creation workflow.The service definitions clearly represent the exercise creation steps, with the
ReviewAndRefineservice containing the implemented functionality for consistency checking and problem statement rewriting.
121-138: Clean and focused request/response message definitions.The messages are appropriately designed for their specific purposes, with clear field naming and proper structure for the gRPC service methods.
| @@ -0,0 +1,138 @@ | |||
| syntax = "proto3"; | |||
|
|
|||
| package de.tum.cit.aet.artemis.hyperion; | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix directory structure to match package declaration.
The static analysis tool correctly identifies that the file location doesn't match the package name. The package de.tum.cit.aet.artemis.hyperion should be in directory de/tum/cit/aet/artemis/hyperion relative to the source root.
Move the file to the correct directory structure:
🏁 Script executed:
#!/bin/bash
# Verify current file location and suggest proper structure
echo "Current file location:"
fd hyperion.proto
echo -e "\nExpected directory structure for package 'de.tum.cit.aet.artemis.hyperion':"
echo "src/main/proto/de/tum/cit/aet/artemis/hyperion/hyperion.proto"Length of output: 445
Relocate hyperion.proto to match its package declaration
The hyperion.proto file is currently at
src/main/java/de/tum/cit/aet/artemis/hyperion/proto/hyperion.proto
but declares
package de.tum.cit.aet.artemis.hyperion;To satisfy the package path, move it to:
src/main/proto/de/tum/cit/aet/artemis/hyperion/hyperion.proto
• Run for example:
git mv \
src/main/java/de/tum/cit/aet/artemis/hyperion/proto/hyperion.proto \
src/main/proto/de/tum/cit/aet/artemis/hyperion/hyperion.proto• Update any build or Gradle Protobuf plugin paths if needed.
🧰 Tools
🪛 Buf (1.54.0)
3-3: Files with package "de.tum.cit.aet.artemis.hyperion" must be within a directory "de/tum/cit/aet/artemis/hyperion" relative to root but were in directory "src/main/java/de/tum/cit/aet/artemis/hyperion/proto".
(PACKAGE_DIRECTORY_MATCH)
🤖 Prompt for AI Agents
In src/main/java/de/tum/cit/aet/artemis/hyperion/proto/hyperion.proto at line 3,
the package declaration is de.tum.cit.aet.artemis.hyperion but the file is
located in a mismatched directory. Move the hyperion.proto file to
src/main/proto/de/tum/cit/aet/artemis/hyperion/hyperion.proto to align with the
package path. After moving, update any build scripts or Gradle Protobuf plugin
configurations that reference the old path to reflect the new location.
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/main/java/de/tum/cit/aet/artemis/hyperion/config/HyperionHealthIndicator.java (1)
61-67: Fix redundant ConnectorHealth creation.This is the same issue previously identified - there's redundant creation of
ConnectorHealthin the unhealthy path. TheconnectorHealthvariable is already assigned on line 59 with the correctisHealthyvalue, making line 66 unnecessary.src/main/java/de/tum/cit/aet/artemis/hyperion/service/HyperionReviewAndRefineService.java (2)
79-79: Remove unreachable return statement.The
return nullstatement is unreachable sincehandleGrpcExceptionthrows an exception. This dead code should be removed to improve clarity.
104-107: Inconsistent exception handling pattern.This method uses a different exception handling pattern compared to
checkConsistency. For consistency, consider using the inheritedhandleGrpcExceptionmethod instead of the specificStatusRuntimeExceptioncatch block.
🧹 Nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/hyperion/service/HyperionReviewAndRefineService.java (1)
42-44: Consider making timeouts configurable.While using constants improves maintainability over inline values, consider making these timeouts configurable through
HyperionConfigurationPropertiesfor better operational flexibility, especially for different deployment environments.Example configuration approach:
private final HyperionConfigurationProperties properties; // Then use: .withDeadlineAfter(properties.getConsistencyCheckTimeoutSeconds(), TimeUnit.SECONDS)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
src/main/resources/config/application-artemis.ymlis excluded by!**/*.ymlsrc/main/resources/config/application-tls.ymlis excluded by!**/*.ymlsrc/main/resources/config/application.ymlis excluded by!**/*.ymlsrc/test/resources/config/application-hyperion.ymlis excluded by!**/*.yml
📒 Files selected for processing (11)
build.gradle(5 hunks)src/main/java/de/tum/cit/aet/artemis/hyperion/config/HyperionConfigurationProperties.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/hyperion/config/HyperionHealthIndicator.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/hyperion/service/AbstractHyperionGrpcService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/hyperion/service/HyperionReviewAndRefineService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/hyperion/web/HyperionReviewAndRefineResource.java(1 hunks)src/main/webapp/i18n/de/health.json(1 hunks)src/main/webapp/i18n/en/health.json(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/HyperionReviewAndRefineIntegrationTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/TestReviewAndRefineService.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/hyperion/config/HyperionTestConfiguration.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/de/tum/cit/aet/artemis/hyperion/config/HyperionConfigurationProperties.java
🚧 Files skipped from review as they are similar to previous changes (6)
- src/main/webapp/i18n/en/health.json
- src/main/webapp/i18n/de/health.json
- src/test/java/de/tum/cit/aet/artemis/hyperion/config/HyperionTestConfiguration.java
- src/main/java/de/tum/cit/aet/artemis/hyperion/service/AbstractHyperionGrpcService.java
- build.gradle
- src/main/java/de/tum/cit/aet/artemis/hyperion/web/HyperionReviewAndRefineResource.java
🧰 Additional context used
📓 Path-based instructions (2)
`src/main/java/**/*.java`: naming:CamelCase; principles:{single_responsibility,s...
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/main/java/de/tum/cit/aet/artemis/hyperion/config/HyperionHealthIndicator.javasrc/main/java/de/tum/cit/aet/artemis/hyperion/service/HyperionReviewAndRefineService.java
`src/test/java/**/*.java`: test_naming: descriptive; test_size: small_specific; ...
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/test/java/de/tum/cit/aet/artemis/hyperion/HyperionReviewAndRefineIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/hyperion/TestReviewAndRefineService.java
🧠 Learnings (5)
📓 Common learnings
Learnt from: magaupp
PR: ls1intum/Artemis#8802
File: src/main/resources/templates/rust/exercise/src/context.rs:1-1
Timestamp: 2024-10-08T15:35:42.972Z
Learning: Code inside the `exercise` directories in the Artemis platform is provided to students and is intended for them to implement. TODO comments in these directories are meant to guide students and should not be addressed in the PR.
Learnt from: magaupp
PR: ls1intum/Artemis#8802
File: src/main/resources/templates/rust/exercise/src/context.rs:1-1
Timestamp: 2024-08-05T00:11:50.650Z
Learning: Code inside the `exercise` directories in the Artemis platform is provided to students and is intended for them to implement. TODO comments in these directories are meant to guide students and should not be addressed in the PR.
Learnt from: magaupp
PR: ls1intum/Artemis#9261
File: src/main/resources/templates/c_plus_plus/test/testUtils/Utils.py:15-35
Timestamp: 2024-10-08T15:35:48.767Z
Learning: In the Artemis project, when adding new programming language templates (e.g., C++), if files like `src/main/resources/templates/c_plus_plus/test/testUtils/Utils.py` are copied from the original C template without significant changes, code style improvements such as adding type annotations are not required in the initial PR introducing them.
Learnt from: magaupp
PR: ls1intum/Artemis#9261
File: src/main/resources/templates/c_plus_plus/test/testUtils/Utils.py:15-35
Timestamp: 2024-10-04T23:53:33.885Z
Learning: In the Artemis project, when adding new programming language templates (e.g., C++), if files like `src/main/resources/templates/c_plus_plus/test/testUtils/Utils.py` are copied from the original C template without significant changes, code style improvements such as adding type annotations are not required in the initial PR introducing them.
Learnt from: alexjoham
PR: ls1intum/Artemis#9455
File: src/test/java/de/tum/cit/aet/artemis/iris/IrisTextExerciseChatMessageIntegrationTest.java:401-401
Timestamp: 2024-10-15T11:33:17.915Z
Learning: In the Artemis project, when new fields are added to classes like `PyrisChatStatusUpdateDTO`, corresponding tests may be implemented in separate integration test classes such as `IrisChatTokenTrackingIntegrationTest`.
src/main/java/de/tum/cit/aet/artemis/hyperion/config/HyperionHealthIndicator.java (4)
Learnt from: Wallenstein61
PR: ls1intum/Artemis#10989
File: src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingConnectorService.java:61-89
Timestamp: 2025-06-06T15:11:12.114Z
Learning: In health monitoring components like HealthStatusWithHistory in SharingConnectorService, it's acceptable to have unsynchronized reader methods (like getLastConnect()) when approximate/eventually consistent reads are sufficient for the use case. The performance benefit of avoiding synchronization overhead outweighs strict consistency requirements for monitoring data.
Learnt from: florian-glombik
PR: ls1intum/Artemis#8858
File: src/main/webapp/app/shared/sidebar/sidebar.component.ts:0-0
Timestamp: 2024-07-09T19:08:59.795Z
Learning: In the context of PR ls1intum/Artemis#8858, avoid suggesting the removal of non-null assertions.
Learnt from: florian-glombik
PR: ls1intum/Artemis#8858
File: src/main/webapp/app/shared/exercise-filter/exercise-filter-modal.component.ts:97-107
Timestamp: 2024-07-09T19:10:33.905Z
Learning: In the context of PR ls1intum/Artemis#8858, avoid suggesting the removal of non-null assertions.
Learnt from: florian-glombik
PR: ls1intum/Artemis#8858
File: src/main/webapp/app/shared/sidebar/sidebar.component.ts:0-0
Timestamp: 2024-10-08T15:35:52.595Z
Learning: In the context of PR ls1intum/Artemis#8858, avoid suggesting the removal of non-null assertions.
src/test/java/de/tum/cit/aet/artemis/hyperion/HyperionReviewAndRefineIntegrationTest.java (17)
Learnt from: alexjoham
PR: ls1intum/Artemis#9455
File: src/test/java/de/tum/cit/aet/artemis/iris/IrisTextExerciseChatMessageIntegrationTest.java:401-401
Timestamp: 2024-10-15T11:33:17.915Z
Learning: In the Artemis project, when new fields are added to classes like `PyrisChatStatusUpdateDTO`, corresponding tests may be implemented in separate integration test classes such as `IrisChatTokenTrackingIntegrationTest`.
Learnt from: magaupp
PR: ls1intum/Artemis#10265
File: src/main/resources/templates/dart/test/test/behavior_test.dart:57-67
Timestamp: 2025-02-06T17:28:16.450Z
Learning: In Artemis test files, reflection is intentionally used to bypass potential implementation errors in constructors and setters, allowing tests to produce meaningful results about core functionality even when students haven't implemented everything correctly.
Learnt from: magaupp
PR: ls1intum/Artemis#9261
File: src/main/resources/templates/c_plus_plus/test/testUtils/AbstractProgramTest.py:23-23
Timestamp: 2024-10-08T15:35:48.767Z
Learning: The file `src/main/resources/templates/c_plus_plus/test/testUtils/AbstractProgramTest.py` was copied from the original C template, and the PR adds no changes to it.
Learnt from: magaupp
PR: ls1intum/Artemis#9261
File: src/main/resources/templates/c_plus_plus/test/testUtils/AbstractProgramTest.py:23-23
Timestamp: 2024-10-04T23:41:59.440Z
Learning: The file `src/main/resources/templates/c_plus_plus/test/testUtils/AbstractProgramTest.py` was copied from the original C template, and the PR adds no changes to it.
Learnt from: magaupp
PR: ls1intum/Artemis#9261
File: src/main/resources/templates/c_plus_plus/test/testUtils/junit/TestSuite.py:19-19
Timestamp: 2024-10-04T23:55:18.437Z
Learning: The file `src/main/resources/templates/c_plus_plus/test/testUtils/junit/TestSuite.py` was copied from the original C template, and the PR adds no changes to it.
Learnt from: magaupp
PR: ls1intum/Artemis#9261
File: src/main/resources/templates/c_plus_plus/test/testUtils/junit/TestSuite.py:19-19
Timestamp: 2024-10-08T15:35:48.767Z
Learning: The file `src/main/resources/templates/c_plus_plus/test/testUtils/junit/TestSuite.py` was copied from the original C template, and the PR adds no changes to it.
Learnt from: magaupp
PR: ls1intum/Artemis#9261
File: src/main/resources/templates/c_plus_plus/test/testUtils/TestFailedError.py:6-6
Timestamp: 2024-10-04T23:23:49.139Z
Learning: In the Artemis project, when adding new programming language templates like C++, files such as `src/main/resources/templates/c_plus_plus/test/testUtils/TestFailedError.py` may be copied from existing templates without modifications. In such cases, suggesting modifications to these files (e.g., removing unnecessary `pass` statements) may not be necessary.
Learnt from: magaupp
PR: ls1intum/Artemis#9751
File: src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java:143-148
Timestamp: 2024-11-26T20:43:17.588Z
Learning: In `src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java`, the test package name assigned in `populateUnreleasedProgrammingExercise` does not need to adhere to naming conventions.
Learnt from: Strohgelaender
PR: ls1intum/Artemis#8574
File: src/main/java/de/tum/in/www1/artemis/service/tutorialgroups/TutorialGroupService.java:0-0
Timestamp: 2024-10-08T15:35:42.972Z
Learning: The `tryToFindMatchingUsers` method in `TutorialGroupService.java` has been updated to skip registrations without a student, enhancing the method's robustness. This change was implemented in commit `bef30f9751de0913143e8cb28cc0088264052261`.
Learnt from: magaupp
PR: ls1intum/Artemis#9261
File: src/main/resources/templates/c_plus_plus/test/testUtils/Utils.py:15-35
Timestamp: 2024-10-04T23:53:33.885Z
Learning: In the Artemis project, when adding new programming language templates (e.g., C++), if files like `src/main/resources/templates/c_plus_plus/test/testUtils/Utils.py` are copied from the original C template without significant changes, code style improvements such as adding type annotations are not required in the initial PR introducing them.
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#10921
File: src/test/java/de/tum/cit/aet/artemis/exam/ExamIntegrationTest.java:1334-1339
Timestamp: 2025-06-15T04:13:22.541Z
Learning: In Artemis ExamIntegrationTest, time difference assertions use ChronoUnit.MILLIS.between().isLessThan(1) without Math.abs() because the server only stores and retrieves timestamp values without calling now(), making differences predictable and consistent due to serialization/storage precision rather than timing variations.
Learnt from: ahmetsenturk
PR: ls1intum/Artemis#10187
File: src/test/java/de/tum/cit/aet/artemis/core/user/AccountResourceIntegrationTest.java:473-503
Timestamp: 2025-01-30T09:46:28.171Z
Learning: In Java integration tests for Artemis, use HibernateQueryInterceptor to track database query counts. Inject the interceptor, call startQueryCount() at the start of the test, and use getQueryCount() to verify the number of queries executed.
Learnt from: Wallenstein61
PR: ls1intum/Artemis#10989
File: src/main/java/de/tum/cit/aet/artemis/programming/web/ExerciseSharingResource.java:122-135
Timestamp: 2025-06-06T14:47:54.300Z
Learning: In Artemis sharing platform integration (under "sharing" profile), endpoints in ExerciseSharingResource use checksum-based authentication with shared secrets instead of @PreAuthorize annotations. These endpoints (like getProblemStatement, getExerciseDetails, loadShoppingBasket) validate security through sharingInfo.checkChecksum() calls rather than role-based access control, as they serve as API interfaces for external sharing platform communication.
Learnt from: magaupp
PR: ls1intum/Artemis#8802
File: src/main/resources/templates/rust/exercise/src/context.rs:1-1
Timestamp: 2024-10-08T15:35:42.972Z
Learning: Code inside the `exercise` directories in the Artemis platform is provided to students and is intended for them to implement. TODO comments in these directories are meant to guide students and should not be addressed in the PR.
Learnt from: magaupp
PR: ls1intum/Artemis#8802
File: src/main/resources/templates/rust/exercise/src/context.rs:1-1
Timestamp: 2024-08-05T00:11:50.650Z
Learning: Code inside the `exercise` directories in the Artemis platform is provided to students and is intended for them to implement. TODO comments in these directories are meant to guide students and should not be addressed in the PR.
Learnt from: magaupp
PR: ls1intum/Artemis#10202
File: src/main/resources/templates/ruby/test/test/test_structural.rb:5-18
Timestamp: 2025-01-25T17:22:31.410Z
Learning: When reviewing exercise templates, avoid adding tests that would pass immediately with the starter code. Tests should verify the actual implementation that students need to complete, not just the presence of classes and methods defined in the template.
Learnt from: valentin-boehm
PR: ls1intum/Artemis#7384
File: src/test/java/de/tum/in/www1/artemis/exam/StudentExamIntegrationTest.java:988-993
Timestamp: 2024-06-10T19:44:09.116Z
Learning: The `testSubmitStudentExam_differentUser` method does not require additional checks to verify the state of `studentExam1` after receiving a `HttpStatus.FORBIDDEN` because the control flow in the `StudentExamResource` is straightforward and ensures no state change occurs.
src/test/java/de/tum/cit/aet/artemis/hyperion/TestReviewAndRefineService.java (6)
Learnt from: alexjoham
PR: ls1intum/Artemis#9455
File: src/test/java/de/tum/cit/aet/artemis/iris/IrisTextExerciseChatMessageIntegrationTest.java:401-401
Timestamp: 2024-10-15T11:33:17.915Z
Learning: In the Artemis project, when new fields are added to classes like `PyrisChatStatusUpdateDTO`, corresponding tests may be implemented in separate integration test classes such as `IrisChatTokenTrackingIntegrationTest`.
Learnt from: magaupp
PR: ls1intum/Artemis#10265
File: src/main/resources/templates/dart/test/test/behavior_test.dart:57-67
Timestamp: 2025-02-06T17:28:16.450Z
Learning: In Artemis test files, reflection is intentionally used to bypass potential implementation errors in constructors and setters, allowing tests to produce meaningful results about core functionality even when students haven't implemented everything correctly.
Learnt from: julian-christl
PR: ls1intum/Artemis#8052
File: src/test/java/de/tum/in/www1/artemis/lecture/CompetencyIntegrationTest.java:310-310
Timestamp: 2024-06-10T19:44:09.116Z
Learning: Modifications to parameters in `competencyProgressUtilService.createCompetencyProgress` for debugging purposes are considered irrelevant to the test outcomes but helpful for clarity during debugging.
Learnt from: magaupp
PR: ls1intum/Artemis#9261
File: src/main/resources/templates/c_plus_plus/test/testUtils/TestFailedError.py:6-6
Timestamp: 2024-10-04T23:23:49.139Z
Learning: In the Artemis project, when adding new programming language templates like C++, files such as `src/main/resources/templates/c_plus_plus/test/testUtils/TestFailedError.py` may be copied from existing templates without modifications. In such cases, suggesting modifications to these files (e.g., removing unnecessary `pass` statements) may not be necessary.
Learnt from: magaupp
PR: ls1intum/Artemis#9751
File: src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java:143-148
Timestamp: 2024-11-26T20:43:17.588Z
Learning: In `src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java`, the test package name assigned in `populateUnreleasedProgrammingExercise` does not need to adhere to naming conventions.
Learnt from: bensofficial
PR: ls1intum/Artemis#9608
File: src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIService.java:202-203
Timestamp: 2024-10-27T11:50:42.955Z
Learning: In `src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIService.java`, exception messages are used internally, and minor inconsistencies are acceptable due to performance considerations.
src/main/java/de/tum/cit/aet/artemis/hyperion/service/HyperionReviewAndRefineService.java (20)
Learnt from: bensofficial
PR: ls1intum/Artemis#9608
File: src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIService.java:202-203
Timestamp: 2024-10-27T11:50:42.955Z
Learning: In `src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIService.java`, exception messages are used internally, and minor inconsistencies are acceptable due to performance considerations.
Learnt from: magaupp
PR: ls1intum/Artemis#8802
File: src/main/resources/templates/rust/exercise/src/context.rs:1-1
Timestamp: 2024-10-08T15:35:42.972Z
Learning: Code inside the `exercise` directories in the Artemis platform is provided to students and is intended for them to implement. TODO comments in these directories are meant to guide students and should not be addressed in the PR.
Learnt from: magaupp
PR: ls1intum/Artemis#8802
File: src/main/resources/templates/rust/exercise/src/context.rs:1-1
Timestamp: 2024-08-05T00:11:50.650Z
Learning: Code inside the `exercise` directories in the Artemis platform is provided to students and is intended for them to implement. TODO comments in these directories are meant to guide students and should not be addressed in the PR.
Learnt from: janthoXO
PR: ls1intum/Artemis#9406
File: src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java:209-209
Timestamp: 2025-02-11T12:05:49.151Z
Learning: In ProgrammingExerciseParticipationResource, exam-related authorization checks and sensitive information filtering for results and feedbacks are handled by resultService.filterSensitiveInformationIfNecessary().
Learnt from: magaupp
PR: ls1intum/Artemis#9751
File: src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java:143-148
Timestamp: 2024-11-26T20:43:17.588Z
Learning: In `src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java`, the test package name assigned in `populateUnreleasedProgrammingExercise` does not need to adhere to naming conventions.
Learnt from: Strohgelaender
PR: ls1intum/Artemis#8574
File: src/main/java/de/tum/in/www1/artemis/service/tutorialgroups/TutorialGroupService.java:0-0
Timestamp: 2024-10-08T15:35:42.972Z
Learning: The `tryToFindMatchingUsers` method in `TutorialGroupService.java` has been updated to skip registrations without a student, enhancing the method's robustness. This change was implemented in commit `bef30f9751de0913143e8cb28cc0088264052261`.
Learnt from: bensofficial
PR: ls1intum/Artemis#9608
File: src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIService.java:218-239
Timestamp: 2024-10-27T11:46:37.477Z
Learning: In `src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIService.java`, within the `generateGitLabTestToken` method, the token expiry is intentionally hardcoded to 6 months to match the semester duration and should not be made configurable.
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#9303
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:296-300
Timestamp: 2024-10-20T18:37:45.365Z
Learning: When reviewing code changes in `StudentExamService.saveSubmission`, if the PR aims to improve readability without changing logic, avoid suggesting changes that alter logic, such as adding exceptions in the default case of switch statements.
Learnt from: magaupp
PR: ls1intum/Artemis#9609
File: src/main/java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/strategy/sarif/SarifParser.java:77-79
Timestamp: 2024-11-10T02:31:09.124Z
Learning: In the codebase, parsers like `SarifParser` should rethrow exceptions as `RuntimeException` to align with other parsers, and introducing new exception types that have no meaning outside the class is not preferred.
Learnt from: florian-glombik
PR: ls1intum/Artemis#8858
File: src/test/javascript/spec/component/shared/exercise-filter/exercise-filter-modal.component.spec.ts:201-225
Timestamp: 2024-10-08T15:35:48.768Z
Learning: Non-null assertions should not be flagged or suggested for removal in the context of PR ls1intum/Artemis#8858.
Learnt from: florian-glombik
PR: ls1intum/Artemis#8858
File: src/test/javascript/spec/component/shared/exercise-filter/exercise-filter-modal.component.spec.ts:201-225
Timestamp: 2024-07-09T19:10:20.372Z
Learning: Non-null assertions should not be flagged or suggested for removal in the context of PR ls1intum/Artemis#8858.
Learnt from: florian-glombik
PR: ls1intum/Artemis#8858
File: src/main/webapp/app/shared/sidebar/sidebar.component.ts:0-0
Timestamp: 2024-07-09T19:08:59.795Z
Learning: In the context of PR ls1intum/Artemis#8858, avoid suggesting the removal of non-null assertions.
Learnt from: florian-glombik
PR: ls1intum/Artemis#8858
File: src/main/webapp/app/shared/exercise-filter/exercise-filter-modal.component.ts:97-107
Timestamp: 2024-07-09T19:10:33.905Z
Learning: In the context of PR ls1intum/Artemis#8858, avoid suggesting the removal of non-null assertions.
Learnt from: florian-glombik
PR: ls1intum/Artemis#8858
File: src/main/webapp/app/shared/sidebar/sidebar.component.ts:0-0
Timestamp: 2024-10-08T15:35:52.595Z
Learning: In the context of PR ls1intum/Artemis#8858, avoid suggesting the removal of non-null assertions.
Learnt from: valentin-boehm
PR: ls1intum/Artemis#7384
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:295-303
Timestamp: 2024-06-10T19:44:09.116Z
Learning: The `abandonStudentExam` method in `StudentExamService` should not include a null check for the `studentExam` parameter as per the project's coding practices. It is expected that the `studentExam` will never be null at this point in the code, and a `NullPointerException` would indicate a significant issue elsewhere in the codebase.
Learnt from: valentin-boehm
PR: ls1intum/Artemis#7384
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:295-303
Timestamp: 2024-10-14T10:30:20.109Z
Learning: The `abandonStudentExam` method in `StudentExamService` should not include a null check for the `studentExam` parameter as per the project's coding practices. It is expected that the `studentExam` will never be null at this point in the code, and a `NullPointerException` would indicate a significant issue elsewhere in the codebase.
Learnt from: florian-glombik
PR: ls1intum/Artemis#10956
File: src/main/java/de/tum/cit/aet/artemis/core/security/passkey/ArtemisPublicKeyCredentialCreationOptionsFilter.java:96-97
Timestamp: 2025-06-01T20:24:10.681Z
Learning: In the Artemis codebase, the AuthorizationManager.authorize() method can return null, making null checks necessary when using this API in classes like ArtemisPublicKeyCredentialCreationOptionsFilter.
Learnt from: iyannsch
PR: ls1intum/Artemis#8965
File: src/main/java/de/tum/in/www1/artemis/config/TheiaConfiguration.java:33-41
Timestamp: 2024-10-08T15:35:52.595Z
Learning: When handling potential null values in server-side methods, consider that the client-side might already have null value handling in place. However, it can still be good practice to handle null values on the server-side for robustness.
Learnt from: iyannsch
PR: ls1intum/Artemis#8965
File: src/main/java/de/tum/in/www1/artemis/config/TheiaConfiguration.java:33-41
Timestamp: 2024-07-09T13:41:25.045Z
Learning: When handling potential null values in server-side methods, consider that the client-side might already have null value handling in place. However, it can still be good practice to handle null values on the server-side for robustness.
Learnt from: Wallenstein61
PR: ls1intum/Artemis#10989
File: src/main/java/de/tum/cit/aet/artemis/programming/web/ExerciseSharingResource.java:122-135
Timestamp: 2025-06-06T14:47:54.300Z
Learning: In Artemis sharing platform integration (under "sharing" profile), endpoints in ExerciseSharingResource use checksum-based authentication with shared secrets instead of @PreAuthorize annotations. These endpoints (like getProblemStatement, getExerciseDetails, loadShoppingBasket) validate security through sharingInfo.checkChecksum() calls rather than role-based access control, as they serve as API interfaces for external sharing platform communication.
🧬 Code Graph Analysis (2)
src/test/java/de/tum/cit/aet/artemis/hyperion/TestReviewAndRefineService.java (2)
src/main/webapp/app/shared/service/base-api-http.service.ts (1)
request(40-70)src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts (1)
getText(211-213)
src/main/java/de/tum/cit/aet/artemis/hyperion/service/HyperionReviewAndRefineService.java (2)
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
Constants(8-503)src/main/java/de/tum/cit/aet/artemis/hyperion/service/AbstractHyperionGrpcService.java (1)
Profile(17-55)
🔇 Additional comments (3)
src/test/java/de/tum/cit/aet/artemis/hyperion/HyperionReviewAndRefineIntegrationTest.java (1)
30-128: Excellent comprehensive test coverage.The integration test class demonstrates best practices with:
- Proper test isolation using
@DirtiesContext- Comprehensive coverage of both HTTP endpoints and direct gRPC calls
- Security testing with role-based access control
- Error scenario validation including timeout and invalid input handling
- Descriptive test names and proper use of
assertThat()assertionssrc/test/java/de/tum/cit/aet/artemis/hyperion/TestReviewAndRefineService.java (1)
24-71: Well-designed mock service implementation.The mock service demonstrates excellent testing practices:
- Comprehensive input validation with proper gRPC error codes
- Realistic response construction that enhances test meaningfulness
- Clear separation of validation logic and response generation
- Predictable behavior that supports reliable integration testing
The documentation clearly explains the testing principles followed, making the service maintainable and understandable.
src/main/java/de/tum/cit/aet/artemis/hyperion/service/HyperionReviewAndRefineService.java (1)
135-163: Robust repository content handling.The repository content fetching logic demonstrates excellent error handling practices:
- Safe exception handling with fallback to empty collections
- Proper logging of errors with context
- Language-specific filtering for template/solution repositories
- Clean separation of concerns between different repository types
There was a problem hiding this comment.
Please open a PR for our Artemis Ansible Collection for the required config changes. Otherwise, this will not be usable on any testserver or production.
Additionally, please tell us what values we will have to set for this to work. Especially how we can generate the required certificates.
Also the client tests fail due to coverage and the server tests have initialization errors. Additionally, you probably want to remove the generated folder from the Jacoco coverage.
|
|
||
| 1. **Setup Hyperion Service** (external dependency) | ||
|
|
||
| - Deploy Hyperion service separately (see Hyperion documentation) |
There was a problem hiding this comment.
Please add a link to the Hyperion documentation here.
| * Timeout for consistency check operations in seconds. | ||
| * This is a business logic timeout, not a gRPC connection timeout. | ||
| */ | ||
| private int consistencyCheckTimeoutSeconds = 300; // 5 minutes |
There was a problem hiding this comment.
I think it would make sense to turn this into a proper config value with 300 being the default. (Yes, I did see the warning in the PR description, but with default values the test server config does not have to contain any specific value for it)
| protected void handleGrpcException(String operation, Exception e) { | ||
| if (e instanceof StatusRuntimeException grpcException) { | ||
| log.error("gRPC call failed for {}: {} (Status: {})", operation, grpcException.getMessage(), grpcException.getStatus().getCode()); | ||
| throw new HyperionServiceException("Failed to execute " + operation, grpcException); | ||
| } | ||
| else { | ||
| log.error("Unexpected error during {}: {}", operation, e.getMessage(), e); | ||
| throw new HyperionServiceException("Unexpected error during " + operation, e); | ||
| } | ||
| } |
There was a problem hiding this comment.
I am not a super big fan of this exception handling. Due to HyperionServiceException being a simple RuntimeException it will result in an normal 500 Internal Server Error for the client. This effectively means that this exception wrapping has no real effect, except for masking the message of the original exception in the clients network tab.
A better approach would be to let HyperionServiceException be a subclass of an HTTP Status code based Exception. These can then be (automatically) properly handled in the client.
| @@ -27,6 +27,7 @@ | |||
| "hazelcast": "Hazelcast", | |||
| "websocketBroker": "Websocket Broker (Server -> Broker)", | |||
| "athena": "Athena", | |||
| "grpcChannel": "Edutelligence (gRPC)", | |||
There was a problem hiding this comment.
Does this work? Shouldn't this have to be like this?
| "grpcChannel": "Edutelligence (gRPC)", | |
| "hyperion": "Hyperion (gRPC)", |
| @SpringBootTest(properties = { "grpc.server.inProcessName=test", "grpc.server.port=-1", "grpc.client.hyperion.address=in-process:test" }) | ||
| @SpringJUnitConfig(classes = { HyperionTestConfiguration.class }) | ||
| @ActiveProfiles({ "test", "artemis", "scheduling", "localci", "localvc", PROFILE_HYPERION }) | ||
| @DirtiesContext |
There was a problem hiding this comment.
This is a bad idea, as this will lead to a new server start in the tests. this would slow down the tests by ~30 seconds. Please instead add the profile to one of the existing abstract integration test classes.
Important
Corresponding Hyperion Service PR #176.
Caution
There are some parts of the configuration hardcoded to make it work without changing the configuration of the test servers. Those still need to be configured correctly and the configuration code needs to be removed.
Checklist
General
Server
Client
Motivation and Context
This PR integrates Hyperion, a new AI-powered microservice from the EduTelligence suite, to enhance programming exercise creation and management. Hyperion replaces Iris functionality for:
Migration Goals:
Related Issues:
Description
Core Integration Components
Server Infrastructure:
Service Architecture:
HyperionReviewAndRefineService: Core service for consistency checks and problem statement rewritingHyperionGrpcConfiguration: Managed gRPC channel with TLS and certificate authentication (mTLS)HyperionHealthIndicator: Health monitoring following Spring Boot standardsAbstractHyperionGrpcService: Base class for shared gRPC functionalitySecurity & Authorization:
@EnforceAtLeastEditorInExercise- Editors and Instructors only@EnforceAtLeastTutor- Tutors, Editors, and Instructors@EnforceAtLeastEditor, in previously coupled FAQ rewriting we wanted Tutor AFAIKClient Updates:
ArtemisIntelligenceServiceto route consistency checks and problem statement rewriting to HyperionMigration Changes
Removed Iris Components:
IrisConsistencyCheckServiceand related DTOsData Flow:
Configuration & Deployment
Development Setup:
Production Setup (with mTLS):
Profile Activation:
hyperionprofile to activate / deactivate the AI programming creation assistanceTesting Strategy
Comprehensive Test Coverage:
Test Infrastructure:
Steps for Testing
Prerequisites:
localhost:50051(development)Consistency Check Testing:
Problem Statement Rewriting Testing:
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Performance Review
Code Review
Manual Tests
Test Coverage
Screenshots
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores
Refactor