Add initial AI feedback module for PDF thesis artifacts#1149
Conversation
…feat/add-initial-feedback-endpoint
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds an experimental AI feedback module for thesis PDF review behind a feature flag. It extracts PDF text and images, runs per-category LLM reviews, merges intermediate findings into one response, and exposes a multipart review endpoint. Configuration, prompts, docs, and tests were updated. ChangesAI Feedback Module
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Claudia-Anthropica
left a comment
Claudia-Anthropica
left a comment
There was a problem hiding this comment.
@bensofficial The AI review pipeline shape is understandable, but this head currently has two failing server tests and the new Spring AI dependencies leave the bundled SBOM stale. I also left comments on request validation and PDF rendering bounds; please fix these before merge.
Claudia-Anthropica
left a comment
Claudia-Anthropica
left a comment
There was a problem hiding this comment.
@bensofficial The SBOM and the failing feedback tests are fixed now; the targeted feedback test package passes locally. I am still requesting changes because the request validation and PDF page-count guard are still missing in this PR, and I replied on those two threads.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (11)
server/src/test/java/de/tum/cit/aet/thesis/feedback/controller/ReviewControllerTest.java (1)
3-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse AssertJ here to match the test conventions.
This test is on the right path, but the assertions should use
assertThat(...)instead of JUnit’sassertEquals/assertArrayEquals.Suggested change
-import static org.junit.jupiter.api.Assertions.assertArrayEquals; -import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.assertj.core.api.Assertions.assertThat; @@ - assertEquals("LOCAL", capturedRequest.providerCategory().name()); - assertEquals("proposal.pdf", capturedRequest.file().getOriginalFilename()); - assertArrayEquals(proposalTemplateContent, capturedRequest.file().getBytes()); + assertThat(capturedRequest.providerCategory().name()).isEqualTo("LOCAL"); + assertThat(capturedRequest.file().getOriginalFilename()).isEqualTo("proposal.pdf"); + assertThat(capturedRequest.file().getBytes()).isEqualTo(proposalTemplateContent);As per path instructions,
server/src/test/java/**/*.java: “Use AssertJ assertions (assertThat) over JUnit assertions”.Also applies to: 83-85
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/test/java/de/tum/cit/aet/thesis/feedback/controller/ReviewControllerTest.java` around lines 3 - 4, The test imports in ReviewControllerTest should follow the AssertJ convention instead of JUnit assertions. Replace the JUnit static imports with AssertJ’s assertThat usage, and update the affected assertions in this test to use assertThat(...) for both array and scalar checks so the test matches the surrounding server test style.Source: Path instructions
server/src/main/java/de/tum/cit/aet/thesis/feedback/service/ReviewService.java (2)
58-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the production TODO or move it to an issue.
As per path instructions, "Avoid leaving TODO comments in production code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/main/java/de/tum/cit/aet/thesis/feedback/service/ReviewService.java` at line 58, Remove the production TODO from ReviewService and replace it with a proper implementation or move the note into an external issue/task tracker. Locate the comment in the review flow around the ReviewService logic and delete the TODO marker so no production code contains leftover planning notes.Source: Path instructions
56-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve review category order through the merge step.
The loop runs
ReviewCategory.values()in enum order, butnew HashMap<>()drops that contract beforebuildMergePrompt(). That makes the final prompt less reproducible and harder to debug. ALinkedHashMap(or building the merge input directly in the loop) would keep the pipeline deterministic.Also applies to: 74-96
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/main/java/de/tum/cit/aet/thesis/feedback/service/ReviewService.java` at line 56, The merge input in ReviewService is losing the enum iteration order because reviewResults is created as a HashMap, which makes buildMergePrompt() non-deterministic. Change the collection used in the ReviewService flow to preserve insertion order, such as a LinkedHashMap, or build the merge input directly while iterating ReviewCategory.values() so the review categories stay in enum order through the merge step.server/src/test/java/de/tum/cit/aet/thesis/feedback/service/ReviewServiceTest.java (2)
70-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename these tests to describe scenario and expected outcome.
testReviewandtestBuildMergePromptare too generic for future failures and reports. As per path instructions, "Descriptive test method names that explain scenario and expected outcome".Also applies to: 98-98
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/test/java/de/tum/cit/aet/thesis/feedback/service/ReviewServiceTest.java` at line 70, Rename the generic test methods in ReviewServiceTest, especially testReview and testBuildMergePrompt, so each name clearly states the scenario and expected outcome. Update the method names to follow the path guidance for descriptive test names, using the existing ReviewServiceTest, testReview, and testBuildMergePrompt methods as the references to locate them.Source: Path instructions
92-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the expected call count from
ReviewCategory.
times(9)bakes the current enum size into the test, so adding or removing a review category will break this assertion even if the production loop is still correct. Verifying againstReviewCategory.values().lengthkeeps the test aligned with the actual contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/test/java/de/tum/cit/aet/thesis/feedback/service/ReviewServiceTest.java` at line 92, The test in ReviewServiceTest hardcodes the llmReviewer.review call count with times(9), which couples it to the current number of ReviewCategory entries. Update the verification to derive the expected count from ReviewCategory.values().length so the assertion stays aligned with the loop in ReviewService and continues to match the contract if categories change.server/src/main/java/de/tum/cit/aet/thesis/feedback/controller/ReviewController.java (1)
46-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the production TODO or track it outside the code.
As per path instructions, "Avoid leaving TODO comments in production code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/main/java/de/tum/cit/aet/thesis/feedback/controller/ReviewController.java` at line 46, Remove the production TODO comment in ReviewController and, if the follow-up is still needed, track it in an external issue instead of leaving it in the code. Update the ReviewController logic so the comment about reusing the already uploaded thesis-service file is either implemented or omitted, keeping the production code free of TODOs.Source: Path instructions
server/src/test/java/de/tum/cit/aet/thesis/feedback/service/PdfServiceTest.java (1)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse AssertJ consistently in this class.
The file already uses
assertThat, but most assertions still go through JUnit. Converting the rest will keep failure output consistent with the repo’s test style. As per path instructions, "Use AssertJ assertions (assertThat) over JUnit assertions".Also applies to: 39-39, 67-74, 82-82, 92-113, 121-121
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/test/java/de/tum/cit/aet/thesis/feedback/service/PdfServiceTest.java` at line 4, In PdfServiceTest, replace the remaining JUnit assertions imported from Assertions with AssertJ-style assertions so the class uses assertThat consistently throughout. Update the assertion blocks in the test methods currently using JUnit helpers to use AssertJ equivalents, keeping the existing test structure intact and ensuring the class only relies on the AssertJ assertion style already present.Source: Path instructions
server/src/main/java/de/tum/cit/aet/thesis/feedback/dto/FindingDTO.java (1)
3-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
@JsonInclude(NON_EMPTY)to this DTO.This record is in the DTO path but currently serializes empty strings and empty collections instead of following the repository’s DTO contract.
Proposed fix
+import com.fasterxml.jackson.annotation.JsonInclude; import java.util.List; +@JsonInclude(JsonInclude.Include.NON_EMPTY) public record FindingDTO(String severity, String category, String title, String description, List<Location> locations) { }As per coding guidelines,
server/**/dto/**/*.java: All DTOs use@JsonInclude(JsonInclude.Include.NON_EMPTY)to omit null, empty strings, and empty collections from JSON. Based on learnings, applyJsonInclude(JsonInclude.Include.NON_EMPTY)to all DTOs to minimize payload size by excluding null or empty values from JSON.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/main/java/de/tum/cit/aet/thesis/feedback/dto/FindingDTO.java` around lines 3 - 5, Add JsonInclude(JsonInclude.Include.NON_EMPTY) to the FindingDTO record so it follows the DTO serialization contract and omits null, empty strings, and empty collections. Update the FindingDTO declaration to include the Jackson annotation, and ensure any required JsonInclude import is added alongside the existing record fields.Sources: Coding guidelines, Learnings
server/src/main/java/de/tum/cit/aet/thesis/feedback/dto/ReviewResultDTO.java (1)
3-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
@JsonInclude(NON_EMPTY)to the response DTO.The response record is missing the DTO serialization rule used elsewhere in the project.
Proposed fix
+import com.fasterxml.jackson.annotation.JsonInclude; import java.util.List; +@JsonInclude(JsonInclude.Include.NON_EMPTY) public record ReviewResultDTO(AssessmentCategory category, String summary, List<FindingDTO> findings) { }As per coding guidelines,
server/**/dto/**/*.java: All DTOs use@JsonInclude(JsonInclude.Include.NON_EMPTY)to omit null, empty strings, and empty collections from JSON. Based on learnings, applyJsonInclude(JsonInclude.Include.NON_EMPTY)to all DTOs to minimize payload size by excluding null or empty values from JSON.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/main/java/de/tum/cit/aet/thesis/feedback/dto/ReviewResultDTO.java` around lines 3 - 5, The ReviewResultDTO record is missing the project-wide JSON serialization rule, so update this DTO to use JsonInclude with NON_EMPTY like the other DTOs. Add the annotation to ReviewResultDTO so its category, summary, and findings fields are omitted from JSON when null or empty, following the same pattern used across server DTOs.Sources: Coding guidelines, Learnings
server/src/main/java/de/tum/cit/aet/thesis/feedback/dto/IntermediateReviewResult.java (1)
3-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
@JsonInclude(NON_EMPTY)to this DTO.This record also misses the DTO-level JSON inclusion rule, so empty
findingspayloads will be serialized instead of following the project convention.Proposed fix
+import com.fasterxml.jackson.annotation.JsonInclude; import java.util.List; +@JsonInclude(JsonInclude.Include.NON_EMPTY) public record IntermediateReviewResult(List<FindingDTO> findings) { }As per coding guidelines,
server/**/dto/**/*.java: All DTOs use@JsonInclude(JsonInclude.Include.NON_EMPTY)to omit null, empty strings, and empty collections from JSON. Based on learnings, applyJsonInclude(JsonInclude.Include.NON_EMPTY)to all DTOs to minimize payload size by excluding null or empty values from JSON.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/main/java/de/tum/cit/aet/thesis/feedback/dto/IntermediateReviewResult.java` around lines 3 - 5, The IntermediateReviewResult DTO is missing the project-wide JSON inclusion rule, so empty findings can still be serialized. Add the DTO-level JsonInclude annotation with NON_EMPTY on the IntermediateReviewResult record, matching the convention used by other DTOs in this package so null and empty values are omitted from JSON output.Sources: Coding guidelines, Learnings
server/src/test/java/de/tum/cit/aet/thesis/feedback/service/reviewer/PromptsTest.java (1)
9-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit this into focused tests and use AssertJ
This test mixes nine valid slugs with the invalid-slug case. A parameterized happy-path test plus a separate invalid-slug test would make failures easier to pinpoint, and
assertThat/assertThatThrownBywould match the rest of the test suite better.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/test/java/de/tum/cit/aet/thesis/feedback/service/reviewer/PromptsTest.java` around lines 9 - 20, The current PromptsTest mixes all valid ReviewCategory.fromSlug mappings with the invalid-slug case in one method, making failures harder to isolate. Split it into a parameterized happy-path test for the known slugs in ReviewCategory.fromSlug and a separate test for the invalid input, and switch the assertions to AssertJ using assertThat for prompt equality and assertThatThrownBy for the exception case.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/src/main/java/de/tum/cit/aet/thesis/feedback/dto/Location.java`:
- Around line 1-4: Add `@JsonInclude`(JsonInclude.Include.NON_EMPTY) to the
Location record so the DTO layer omits null and empty values from JSON. Update
the Location type itself in the dto package, following the same pattern used by
other server DTOs, and ensure page, section, and quote are excluded when absent
rather than serialized as empty payload fields.
In
`@server/src/main/java/de/tum/cit/aet/thesis/feedback/dto/ReviewRequestDTO.java`:
- Around line 3-5: ReviewRequestDTO should enforce that both providerCategory
and file are present at the request boundary instead of allowing null binding.
Add bean validation constraints to the ReviewRequestDTO record components, and
ensure the controller path that consumes ReviewRequestDTO triggers validation so
missing multipart fields fail fast with a 400. Use the ReviewRequestDTO record
itself and the request-handling endpoint that accepts it to locate the fix.
In `@server/src/main/java/de/tum/cit/aet/thesis/feedback/service/PdfService.java`:
- Around line 41-54: The PDF parsing path in PdfService.extractTextFromPdf
currently turns unreadable or non-PDF uploads into a generic RuntimeException,
which will surface as a server error instead of a client error. Update the error
handling around file bytes conversion and the PagePdfDocumentReader
reader.read() call to throw or preserve a dedicated bad-request type that
controller advice can map to 400, and keep the distinction clear in any related
Loader.loadPDF path as well. Use the PdfService.extractTextFromPdf and
PagePdfDocumentReader symbols to locate the fix.
In
`@server/src/main/java/de/tum/cit/aet/thesis/feedback/service/reviewer/LlmReviewer.java`:
- Around line 54-60: The review flow in LlmReviewer.review is sending the entire
document text and all page images in one chatClient call, which will not scale
for larger PDFs. Update review() to process pages in bounded chunks or windows
and invoke the model per batch instead of building one pagesText for the full
list. Keep the existing prompt construction (sharedPrompt, taskPrompt,
guidelinesPrompt) but apply it per batch, then merge the
IntermediateReviewResult outputs into a final result for the caller.
In
`@server/src/main/java/de/tum/cit/aet/thesis/feedback/service/reviewer/Prompts.java`:
- Around line 117-129: The proposal length guidance in Prompts is inconsistent
because it says both “around 5-7 pages” and “at most 4-6 text pages.” Update the
proposal instructions to a single, non-conflicting range in the relevant prompt
text so the length requirement is clear and stable; adjust the wording near the
proposal overview and the “Proposal Structure” section to match one consistent
target length.
In
`@server/src/main/java/de/tum/cit/aet/thesis/feedback/service/ReviewService.java`:
- Around line 67-68: The merger prompt in ReviewService.buildMergePrompt and the
chatClient.prompt() call is treating intermediate findings as raw prompt text,
which lets title, description, and especially quote influence the second LLM
call. Update the flow so reviewResults are serialized and passed as structured
JSON or otherwise fully escaped/fenced content, and adjust Prompts.MERGER to
explicitly instruct the model to treat all intermediate fields as data only, not
instructions.
In `@server/src/main/resources/application.yml`:
- Around line 59-65: The shared OpenAI configuration in application.yml
hardcodes a real external base-url, so any environment enabling ai features will
send data there by default. Move the concrete endpoint out of the shared
ai.openai.base-url setting into a local/dev-only profile file, and keep the
shared application.yml env-driven so the endpoint must be explicitly provided
via OPENAI_BASE_URL. Update the ai.openai section to use only environment
variables in the shared config, and preserve the existing OPENAI_API_KEY and
model/temperature pattern.
In
`@server/src/test/java/de/tum/cit/aet/thesis/feedback/service/PdfServiceTest.java`:
- Around line 106-110: The snapshot helper in PdfServiceTest should not create
or write missing files; keep assertImageMatchesSnapshot read-only by removing
the Files.createDirectories/Files.write path and instead failing immediately
with a clear message when the expected snapshot is absent. Update the logic in
assertImageMatchesSnapshot so the test only compares against existing fixtures
under src/test/resources and never mutates them.
---
Nitpick comments:
In
`@server/src/main/java/de/tum/cit/aet/thesis/feedback/controller/ReviewController.java`:
- Line 46: Remove the production TODO comment in ReviewController and, if the
follow-up is still needed, track it in an external issue instead of leaving it
in the code. Update the ReviewController logic so the comment about reusing the
already uploaded thesis-service file is either implemented or omitted, keeping
the production code free of TODOs.
In `@server/src/main/java/de/tum/cit/aet/thesis/feedback/dto/FindingDTO.java`:
- Around line 3-5: Add JsonInclude(JsonInclude.Include.NON_EMPTY) to the
FindingDTO record so it follows the DTO serialization contract and omits null,
empty strings, and empty collections. Update the FindingDTO declaration to
include the Jackson annotation, and ensure any required JsonInclude import is
added alongside the existing record fields.
In
`@server/src/main/java/de/tum/cit/aet/thesis/feedback/dto/IntermediateReviewResult.java`:
- Around line 3-5: The IntermediateReviewResult DTO is missing the project-wide
JSON inclusion rule, so empty findings can still be serialized. Add the
DTO-level JsonInclude annotation with NON_EMPTY on the IntermediateReviewResult
record, matching the convention used by other DTOs in this package so null and
empty values are omitted from JSON output.
In
`@server/src/main/java/de/tum/cit/aet/thesis/feedback/dto/ReviewResultDTO.java`:
- Around line 3-5: The ReviewResultDTO record is missing the project-wide JSON
serialization rule, so update this DTO to use JsonInclude with NON_EMPTY like
the other DTOs. Add the annotation to ReviewResultDTO so its category, summary,
and findings fields are omitted from JSON when null or empty, following the same
pattern used across server DTOs.
In
`@server/src/main/java/de/tum/cit/aet/thesis/feedback/service/ReviewService.java`:
- Line 58: Remove the production TODO from ReviewService and replace it with a
proper implementation or move the note into an external issue/task tracker.
Locate the comment in the review flow around the ReviewService logic and delete
the TODO marker so no production code contains leftover planning notes.
- Line 56: The merge input in ReviewService is losing the enum iteration order
because reviewResults is created as a HashMap, which makes buildMergePrompt()
non-deterministic. Change the collection used in the ReviewService flow to
preserve insertion order, such as a LinkedHashMap, or build the merge input
directly while iterating ReviewCategory.values() so the review categories stay
in enum order through the merge step.
In
`@server/src/test/java/de/tum/cit/aet/thesis/feedback/controller/ReviewControllerTest.java`:
- Around line 3-4: The test imports in ReviewControllerTest should follow the
AssertJ convention instead of JUnit assertions. Replace the JUnit static imports
with AssertJ’s assertThat usage, and update the affected assertions in this test
to use assertThat(...) for both array and scalar checks so the test matches the
surrounding server test style.
In
`@server/src/test/java/de/tum/cit/aet/thesis/feedback/service/PdfServiceTest.java`:
- Line 4: In PdfServiceTest, replace the remaining JUnit assertions imported
from Assertions with AssertJ-style assertions so the class uses assertThat
consistently throughout. Update the assertion blocks in the test methods
currently using JUnit helpers to use AssertJ equivalents, keeping the existing
test structure intact and ensuring the class only relies on the AssertJ
assertion style already present.
In
`@server/src/test/java/de/tum/cit/aet/thesis/feedback/service/reviewer/PromptsTest.java`:
- Around line 9-20: The current PromptsTest mixes all valid
ReviewCategory.fromSlug mappings with the invalid-slug case in one method,
making failures harder to isolate. Split it into a parameterized happy-path test
for the known slugs in ReviewCategory.fromSlug and a separate test for the
invalid input, and switch the assertions to AssertJ using assertThat for prompt
equality and assertThatThrownBy for the exception case.
In
`@server/src/test/java/de/tum/cit/aet/thesis/feedback/service/ReviewServiceTest.java`:
- Line 70: Rename the generic test methods in ReviewServiceTest, especially
testReview and testBuildMergePrompt, so each name clearly states the scenario
and expected outcome. Update the method names to follow the path guidance for
descriptive test names, using the existing ReviewServiceTest, testReview, and
testBuildMergePrompt methods as the references to locate them.
- Line 92: The test in ReviewServiceTest hardcodes the llmReviewer.review call
count with times(9), which couples it to the current number of ReviewCategory
entries. Update the verification to derive the expected count from
ReviewCategory.values().length so the assertion stays aligned with the loop in
ReviewService and continues to match the contract if categories change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 82837810-8524-42f0-8011-e0525c30167b
⛔ Files ignored due to path filters (6)
server/src/test/resources/pdfs/hello-world.pdfis excluded by!**/*.pdf,!**/*.pdfserver/src/test/resources/pdfs/pdf-images/proposal-template-page-1.pngis excluded by!**/*.png,!**/*.pngserver/src/test/resources/pdfs/pdf-images/proposal-template-page-2.pngis excluded by!**/*.png,!**/*.pngserver/src/test/resources/pdfs/pdf-images/proposal-template-page-3.pngis excluded by!**/*.png,!**/*.pngserver/src/test/resources/pdfs/pdf-images/proposal-template-page-4.pngis excluded by!**/*.png,!**/*.pngserver/src/test/resources/pdfs/proposal-template.pdfis excluded by!**/*.pdf,!**/*.pdf
📒 Files selected for processing (27)
docs/CONFIGURATION.mddocs/DEVELOPMENT.mdserver/build.gradleserver/sbom/.input-hashserver/sbom/bom.jsonserver/src/main/java/de/tum/cit/aet/thesis/feedback/config/AIFeaturesEnabled.javaserver/src/main/java/de/tum/cit/aet/thesis/feedback/controller/ReviewController.javaserver/src/main/java/de/tum/cit/aet/thesis/feedback/dto/AssessmentCategory.javaserver/src/main/java/de/tum/cit/aet/thesis/feedback/dto/FindingDTO.javaserver/src/main/java/de/tum/cit/aet/thesis/feedback/dto/IntermediateReviewResult.javaserver/src/main/java/de/tum/cit/aet/thesis/feedback/dto/Location.javaserver/src/main/java/de/tum/cit/aet/thesis/feedback/dto/ProviderCategory.javaserver/src/main/java/de/tum/cit/aet/thesis/feedback/dto/ReviewRequestDTO.javaserver/src/main/java/de/tum/cit/aet/thesis/feedback/dto/ReviewResultDTO.javaserver/src/main/java/de/tum/cit/aet/thesis/feedback/service/PdfService.javaserver/src/main/java/de/tum/cit/aet/thesis/feedback/service/ReviewService.javaserver/src/main/java/de/tum/cit/aet/thesis/feedback/service/reviewer/LlmReviewer.javaserver/src/main/java/de/tum/cit/aet/thesis/feedback/service/reviewer/Prompts.javaserver/src/main/java/de/tum/cit/aet/thesis/feedback/service/reviewer/ReviewCategory.javaserver/src/main/resources/application-dev.ymlserver/src/main/resources/application.ymlserver/src/test/java/de/tum/cit/aet/thesis/feedback/controller/ReviewControllerTest.javaserver/src/test/java/de/tum/cit/aet/thesis/feedback/service/PdfServiceTest.javaserver/src/test/java/de/tum/cit/aet/thesis/feedback/service/ReviewServiceTest.javaserver/src/test/java/de/tum/cit/aet/thesis/feedback/service/reviewer/PromptsTest.javaserver/src/test/resources/application.ymlserver/src/test/resources/pdfs/text.md
Claudia-Anthropica
left a comment
Claudia-Anthropica
left a comment
There was a problem hiding this comment.
@bensofficial The validation, SBOM, and previously failing feedback tests look good now; I ran the targeted feedback package locally and it passes. I’m still requesting changes because the PDF page-count guard is set to 120 pages, which is too loose for this proposal endpoint; I reopened and replied on that thread.
Claudia-Anthropica
left a comment
Claudia-Anthropica
left a comment
There was a problem hiding this comment.
@bensofficial All requested feedback is settled now; I’m accepting the 120-page cap as the current constraint for thesis-sized uploads. Build/test/security checks are passing, with only the dev-container deploy job still pending, so approving.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/src/main/java/de/tum/cit/aet/thesis/feedback/service/ReviewService.java (2)
69-70: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not log full intermediate findings.
intermediateResultcontains model output derived from uploaded thesis PDFs, including quoted document text. Persisting that at debug level leaks sensitive manuscript content into application logs.Suggested fix
- log.debug("Review result for category {}: {}", category.getSlug(), intermediateResult); + log.debug("Review result for category {} contains {} findings", category.getSlug(), intermediateResult.findings().size());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/main/java/de/tum/cit/aet/thesis/feedback/service/ReviewService.java` around lines 69 - 70, The debug log in ReviewService.review is leaking sensitive model output because it prints the full IntermediateReviewResult, which may include quoted thesis content. Remove the object dump from the log and replace it with a minimal summary using safe identifiers like category.getSlug() and non-sensitive metadata from intermediateResult, so the method still logs progress without exposing manuscript text.
63-64: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve category order in the merger input.
Line 63 uses a
HashMap, but this prompt is built from an ordered enum walk. Switching toLinkedHashMapkeeps the merger input stable across runs instead of relying on map iteration order. (docs.oracle.com)Suggested fix
- Map<String, IntermediateReviewResult> reviewResults = new HashMap<>(); + Map<String, IntermediateReviewResult> reviewResults = new LinkedHashMap<>();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/main/java/de/tum/cit/aet/thesis/feedback/service/ReviewService.java` around lines 63 - 64, The merger input in ReviewService should preserve the enum traversal order instead of using an unordered HashMap. Update the reviewResults collection in ReviewService to use LinkedHashMap so the category sequence stays stable across runs and the prompt assembly remains deterministic.
🧹 Nitpick comments (1)
server/src/main/java/de/tum/cit/aet/thesis/feedback/service/ReviewService.java (1)
65-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the production TODO.
Please track this outside the code or replace it with a concrete issue reference. As per path instructions, "Avoid leaving TODO comments in production code."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/main/java/de/tum/cit/aet/thesis/feedback/service/ReviewService.java` at line 65, The TODO comment in ReviewService should not remain in production code. Remove the TODO from the review process code path and, if needed, replace it with a concrete issue reference or tracking note outside the source; use the ReviewService review flow as the target area to locate and clean up the comment.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/src/main/java/de/tum/cit/aet/thesis/feedback/service/PdfService.java`:
- Around line 104-108: The page-limit check in PdfService.assertWithinPageLimit
only enforces the maximum page count, so empty PDFs still pass through. Update
this helper to reject pageCount values of 0 (or less, if applicable) with a
ResourceInvalidParametersException, alongside the existing MAX_PAGES upper-bound
validation. Keep the fix localized in PdfService so ReviewService.review() and
the extractors never receive zero-page input.
---
Outside diff comments:
In
`@server/src/main/java/de/tum/cit/aet/thesis/feedback/service/ReviewService.java`:
- Around line 69-70: The debug log in ReviewService.review is leaking sensitive
model output because it prints the full IntermediateReviewResult, which may
include quoted thesis content. Remove the object dump from the log and replace
it with a minimal summary using safe identifiers like category.getSlug() and
non-sensitive metadata from intermediateResult, so the method still logs
progress without exposing manuscript text.
- Around line 63-64: The merger input in ReviewService should preserve the enum
traversal order instead of using an unordered HashMap. Update the reviewResults
collection in ReviewService to use LinkedHashMap so the category sequence stays
stable across runs and the prompt assembly remains deterministic.
---
Nitpick comments:
In
`@server/src/main/java/de/tum/cit/aet/thesis/feedback/service/ReviewService.java`:
- Line 65: The TODO comment in ReviewService should not remain in production
code. Remove the TODO from the review process code path and, if needed, replace
it with a concrete issue reference or tracking note outside the source; use the
ReviewService review flow as the target area to locate and clean up the comment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 65655432-3e30-43ba-86f1-9d5a7093c4cd
📒 Files selected for processing (9)
server/src/main/java/de/tum/cit/aet/thesis/feedback/controller/ReviewController.javaserver/src/main/java/de/tum/cit/aet/thesis/feedback/dto/FindingDTO.javaserver/src/main/java/de/tum/cit/aet/thesis/feedback/dto/Location.javaserver/src/main/java/de/tum/cit/aet/thesis/feedback/dto/ReviewRequestDTO.javaserver/src/main/java/de/tum/cit/aet/thesis/feedback/dto/ReviewResultDTO.javaserver/src/main/java/de/tum/cit/aet/thesis/feedback/service/PdfService.javaserver/src/main/java/de/tum/cit/aet/thesis/feedback/service/ReviewService.javaserver/src/main/java/de/tum/cit/aet/thesis/feedback/service/reviewer/Prompts.javaserver/src/test/java/de/tum/cit/aet/thesis/feedback/service/ReviewServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (7)
- server/src/main/java/de/tum/cit/aet/thesis/feedback/dto/FindingDTO.java
- server/src/main/java/de/tum/cit/aet/thesis/feedback/dto/Location.java
- server/src/main/java/de/tum/cit/aet/thesis/feedback/dto/ReviewResultDTO.java
- server/src/main/java/de/tum/cit/aet/thesis/feedback/dto/ReviewRequestDTO.java
- server/src/test/java/de/tum/cit/aet/thesis/feedback/service/ReviewServiceTest.java
- server/src/main/java/de/tum/cit/aet/thesis/feedback/service/reviewer/Prompts.java
- server/src/main/java/de/tum/cit/aet/thesis/feedback/controller/ReviewController.java
Claudia-Anthropica
left a comment
Claudia-Anthropica
left a comment
There was a problem hiding this comment.
@bensofficial The latest fixes address the previous threads and the targeted feedback tests pass locally. I found one remaining prompt-injection gap in the first reviewer pass; see the inline comment.
Claudia-Anthropica
left a comment
Claudia-Anthropica
left a comment
There was a problem hiding this comment.
@bensofficial The latest fix addresses the LlmReviewer prompt-injection thread, and the focused test passes locally. The build/test/security checks are passing; only the dev-container deploy job is still pending, so I’m approving.
Claudia-Anthropica
left a comment
Claudia-Anthropica
left a comment
There was a problem hiding this comment.
@bensofficial The dependency/SBOM refresh looks fine; I rechecked the feedback module and the calendar paths affected by the ical4j bump. Focused local tests passed, and GitHub build/test/e2e are green with only deploy and JavaScript/TypeScript CodeQL still pending, so approving.
2 participants
Summary
Adds an experimental, feature-flagged AI feedback module that reviews uploaded thesis PDFs (initially proposals) by chunking the document into per-category LLM passes and merging the findings into a single structured result.
POST /v2/ai-review/review-proposalendpoint (multipart upload, restricted toadmin/advisor/supervisor) returning aReviewResultDTOwith findings (severity, title, category, description, page/section/quote locations).ReviewServiceruns eachReviewCategorythroughLlmReviewerand merges intermediate results via a finalPrompts.MERGERcall against Spring AI'sChatClient.PdfServiceextracts per-page text and renders per-page images so the LLM gets both modalities.Condition(AIFeaturesEnabled→thesis-management.ai.enabled). When off, the controller and services aren't registered and the endpoint returns 404. Off by default inapplication.yml; enabled inapplication-dev.ymlfor local dev only.spring-ai-starter-model-openai) inserver/build.gradle. Configurable viaOPENAI_API_KEY/OPENAI_BASE_URL/OPENAI_CHAT_MODEL— defaults point at the chair's TUM GPU endpoint.docs/CONFIGURATION.mdand the experimental status / dev setup indocs/DEVELOPMENT.md.ReviewController(with@TestPropertySourceflipping the flag on),ReviewService,PdfService, andPrompts, plus fixture PDFs/images underserver/src/test/resources/pdfs.Test plan
cd server && ./gradlew test— including the newfeedbackpackage testscd server && ./gradlew spotlessApply— no formatting driftPOST /v2/ai-review/review-proposalreturns 404proposal-template.pdfand verify a populatedReviewResultDTOcomes back🤖 Generated with Claude Code
Summary by CodeRabbit
POST /v2/ai-review/review-proposal, returning structured review results (category, summary, findings with per-page locations/images).