feat(server): add practice findings REST API for contributor dashboard - #910
Conversation
Add 4 read-only workspace-scoped endpoints to expose practice detection findings to the contributor dashboard. Includes paginated list with filtering, per-practice summary aggregation, single finding detail, and per-PR finding views. - New PracticeFindingController with @WorkspaceScopedController - New PracticeFindingService with consistent auth via getCurrentUser() - 4 JPQL queries with JOIN FETCH for eager loading - Ownership enforced in SQL (not Java) for findByIdAndContributorAndWorkspace - DTOs: PracticeFindingListDTO, PracticeFindingDetailDTO, ContributorPracticeSummaryDTO - Evidence field uses Object (not JsonNode) matching AgentJobDTO pattern - 28 integration tests: auth, pagination, filtering, workspace isolation, IDOR - Architecture test updated to allow PracticeFindingController - OpenAPI spec + client regenerated Closes #896 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdded a read-only Practice Findings API: four GET endpoints for paginated contributor findings, per-practice summaries, single finding detail, and pull-request-scoped findings, plus repository queries, DTOs, service/controller, integration tests, OpenAPI specs, and generated frontend SDK/types. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (webapp)
participant Controller as PracticeFindingController
participant Service as PracticeFindingService
participant Repo as PracticeFindingRepository
Client->>Controller: GET /workspaces/{slug}/practices/findings?...
Controller->>Service: getFindings(workspaceId, filters, pageable)
Service->>Repo: findByContributorAndWorkspace(..., pageable)
Repo-->>Service: Page<PracticeFinding>
Service-->>Controller: Page<PracticeFindingListDTO>
Controller-->>Client: 200 PagePracticeFindingList (JSON)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
github-actions
Bot
added
application-server
There was a problem hiding this comment.
Pull request overview
Adds a read-only “practice findings” API surface to the application-server so the webapp’s contributor dashboard can list, summarize, and drill into practice detection findings in a workspace-scoped way.
Changes:
- Introduces a new workspace-scoped
PracticeFindingController+PracticeFindingServiceproviding 4 GET endpoints (list, summary, detail, PR findings). - Extends
PracticeFindingRepositorywith JPQL read queries (pagination + aggregation + ownership-scoped lookup). - Regenerates OpenAPI + webapp OpenAPI TS client/types/transformers to consume the new endpoints, and adds integration tests + arch rule update.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/PracticeFindingController.java |
New REST controller exposing 4 workspace-scoped read endpoints with pagination/filtering. |
server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/PracticeFindingService.java |
New read-only service using UserRepository.getCurrentUser() for contributor scoping and 404-on-non-owner detail access. |
server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/PracticeFindingRepository.java |
Adds read JPQL queries (paged list w/ countQuery, per-practice summary projection, ownership-scoped detail fetch, PR findings list). |
server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/dto/PracticeFindingListDTO.java |
New list DTO mapping from entity and omitting internal/large fields. |
server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/dto/PracticeFindingDetailDTO.java |
New detail DTO including guidance/reasoning/evidence while omitting internal fields. |
server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/dto/ContributorPracticeSummaryDTO.java |
New summary DTO for per-practice aggregates. |
server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/dto/ContributorPracticeSummaryProjection.java |
New Spring Data projection interface for aggregation query results. |
server/application-server/src/test/java/de/tum/in/www1/hephaestus/architecture/ActivityModuleBoundaryTest.java |
Updates architecture rule to allow the new controller in the practices module. |
server/application-server/src/test/java/de/tum/in/www1/hephaestus/practices/finding/PracticeFindingControllerIntegrationTest.java |
Adds integration test coverage for list/summary/detail/PR endpoints incl. filtering, pagination normalization, and IDOR behavior. |
server/application-server/openapi.yaml |
Regenerated OpenAPI spec with new paths/tags/schemas. |
webapp/src/api/types.gen.ts |
Regenerated client types for new schemas/endpoints. |
webapp/src/api/transformers.gen.ts |
Regenerated response transformers (notably date parsing for detectedAt / lastFindingAt). |
webapp/src/api/sdk.gen.ts |
Regenerated SDK functions for new endpoints. |
webapp/src/api/index.ts |
Regenerated public exports for new SDK functions and types. |
webapp/src/api/@tanstack/react-query.gen.ts |
Regenerated TanStack Query option/key helpers for the new endpoints. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/application-server/openapi.yaml`:
- Around line 3333-3367: The ContributorPracticeSummary schema is missing the
recent verdict trajectory required by the summary endpoint: update the source
DTO/projection that populates ContributorPracticeSummary (add a trajectory field
containing the recent verdict/time series data), update any mapping code that
builds ContributorPracticeSummary instances, then run the project’s
OpenAPI/client generation commands to regenerate
server/application-server/openapi.yaml and all derived client and server
artifacts rather than hand-editing the generated file; ensure the new trajectory
property is included in the generated ContributorPracticeSummary contract and
returned by the summary endpoint.
- Around line 1858-1871: Update the OpenAPI parameter metadata for the query
parameters named "page" and "size" so the contract includes bounds: add
"minimum: 0" for the "page" parameter and for "size" set "minimum: 1", "maximum:
100" while keeping the default of 20; do this in the source used to generate
server/application-server/openapi.yaml (not by hand-editing the generated file),
then run the project's OpenAPI/contract generation command to regenerate
server/application-server/openapi.yaml so clients/docs reflect the limits.
- Line 1879: Remove the empty `@SecurityRequirements` annotations that are
producing security: [] in the generated OpenAPI (seen at lines producing
security: []) so the endpoints inherit the global bearerAuth; specifically open
the controller containing the annotated endpoints and delete the
`@SecurityRequirements` annotation from each affected handler method (the methods
currently producing security: []), then regenerate the OpenAPI spec so
`@OpenAPIDefinition`'s bearerAuth is applied; ensure no other method-level
`@SecurityRequirements` remain and run the spec generation to confirm security:
bearerAuth is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8b8531f-5c88-4884-aa8e-2af8bf1ecb8c
📒 Files selected for processing (15)
server/application-server/openapi.yamlserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/PracticeFindingController.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/PracticeFindingRepository.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/PracticeFindingService.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/dto/ContributorPracticeSummaryDTO.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/dto/ContributorPracticeSummaryProjection.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/dto/PracticeFindingDetailDTO.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/dto/PracticeFindingListDTO.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/architecture/ActivityModuleBoundaryTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/practices/finding/PracticeFindingControllerIntegrationTest.javawebapp/src/api/@tanstack/react-query.gen.tswebapp/src/api/index.tswebapp/src/api/sdk.gen.tswebapp/src/api/transformers.gen.tswebapp/src/api/types.gen.ts
| - in: query | ||
| name: page | ||
| required: false | ||
| schema: | ||
| type: integer | ||
| format: int32 | ||
| default: 0 | ||
| - in: query | ||
| name: size | ||
| required: false | ||
| schema: | ||
| type: integer | ||
| format: int32 | ||
| default: 20 |
There was a problem hiding this comment.
Document the pagination bounds here too.
The PR objective calls out page >= 0 and a default size of 20 with a maximum of 100, but Lines 1858-1871 only describe the defaults. Generated clients and API docs will not reflect the actual request limits. Add the bounds in the source parameter annotations and regenerate the contract. As per coding guidelines, "Never hand-edit generated artifacts in server/application-server/openapi.yaml, webapp/src/api/**/*, server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/**, server/intelligence-service/openapi.yaml, server/intelligence-service/src/shared/db/schema.ts, and docs/contributor/erd/schema.mmd; regenerate instead using the appropriate generation commands".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/application-server/openapi.yaml` around lines 1858 - 1871, Update the
OpenAPI parameter metadata for the query parameters named "page" and "size" so
the contract includes bounds: add "minimum: 0" for the "page" parameter and for
"size" set "minimum: 1", "maximum: 100" while keeping the default of 20; do this
in the source used to generate server/application-server/openapi.yaml (not by
hand-editing the generated file), then run the project's OpenAPI/contract
generation command to regenerate server/application-server/openapi.yaml so
clients/docs reflect the limits.
| schema: | ||
| $ref: "#/components/schemas/PagePracticeFindingList" | ||
| description: Paginated findings returned | ||
| security: [] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the OpenAPI file around the mentioned lines
head -n 1970 server/application-server/openapi.yaml | tail -n 100Repository: ls1intum/Hephaestus
Length of output: 2984
🏁 Script executed:
# Search for the controller file containing these practice findings endpoints
find server -name "*.java" -type f | xargs grep -l "practices/findings" | head -5Repository: ls1intum/Hephaestus
Length of output: 307
🏁 Script executed:
# Also look for controller files containing "PracticeFinding" references
find server -name "*.java" -type f | xargs grep -l "PracticeFinding" | head -10Repository: ls1intum/Hephaestus
Length of output: 1208
🏁 Script executed:
# Read the PracticeFindingController.java to check security annotations
cat -n server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/PracticeFindingController.javaRepository: ls1intum/Hephaestus
Length of output: 6933
🏁 Script executed:
# Search for how `@SecurityRequirements` is used elsewhere in the codebase
rg -A2 "@SecurityRequirements" server/application-server/src/main/java --type javaRepository: ls1intum/Hephaestus
Length of output: 12167
🏁 Script executed:
# Check if there are any controllers that properly declare security requirements
rg -A2 "@SecurityRequirement\(" server/application-server/src/main/java --type javaRepository: ls1intum/Hephaestus
Length of output: 418
🏁 Script executed:
# Check the OpenAPIConfiguration to understand the security setup
cat -n server/application-server/src/main/java/de/tum/in/www1/hephaestus/OpenAPIConfiguration.java | head -100Repository: ls1intum/Hephaestus
Length of output: 4931
🏁 Script executed:
# Check if there are any endpoints that properly use `@SecurityRequirement` instead of empty `@SecurityRequirements`
rg "@SecurityRequirement\(name\s*=\s*" server/application-server/src/main/java --type java | head -20Repository: ls1intum/Hephaestus
Length of output: 215
🏁 Script executed:
# Look at the git history or comments around `@SecurityRequirements` usage to understand the pattern
rg -B5 -A2 "@SecurityRequirements" server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceController.java --type javaRepository: ls1intum/Hephaestus
Length of output: 1179
🏁 Script executed:
# Check the semantics: what does `@SecurityRequirements` with no arguments do?
# Look for any comments or documentation in the codebase about this
rg -i "SecurityRequirements" server/application-server/src/main/java --type java -A5 | grep -i "comment\|doc\|clear\|override" | head -20Repository: ls1intum/Hephaestus
Length of output: 45
🏁 Script executed:
# Search for any controllers that DON'T use `@SecurityRequirements` to see if there's a pattern
rg "@GetMapping" server/application-server/src/main/java -A3 --type java | grep -v "@SecurityRequirements" | head -30Repository: ls1intum/Hephaestus
Length of output: 3742
🏁 Script executed:
# Check the test file to see what auth is actually required at runtime
head -100 server/application-server/src/test/java/de/tum/in/www1/hephaestus/practices/finding/PracticeFindingControllerIntegrationTest.javaRepository: ls1intum/Hephaestus
Length of output: 4062
🏁 Script executed:
# Check if tests for these endpoints require authentication
grep -A 20 "listFindings\|getSummary\|getFinding" server/application-server/src/test/java/de/tum/in/www1/hephaestus/practices/finding/PracticeFindingControllerIntegrationTest.java | grep -E "@WithUser|webTestClient|401|403|200" | head -30Repository: ls1intum/Hephaestus
Length of output: 45
🏁 Script executed:
# Search for how `@SecurityRequirements` empty annotation is documented in springdoc-openapi
rg -i "empty.*security\|security.*clear\|override.*auth" server --type java | head -20Repository: ls1intum/Hephaestus
Length of output: 45
🏁 Script executed:
# Check the OpenAPI v3 spec to understand if empty `@SecurityRequirements` overrides global security
web_searchRepository: ls1intum/Hephaestus
Length of output: 113
🌐 Web query:
@SecurityRequirements empty annotation OpenAPI 3 swagger-annotations override global security
💡 Result:
In OpenAPI 3.0, global security requirements defined at the root level (via security in @OpenAPIDefinition or equivalent) can be overridden at the operation level. To completely disable (remove) global security for a specific operation, use an empty security array []. This overrides and removes the top-level security declaration for that operation. In swagger-annotations (io.swagger.v3.oas.annotations), use @SecurityRequirements (the container for repeatable @SecurityRequirement) with no @SecurityRequirement values, i.e., @SecurityRequirements. This generates an empty security array [] in the operation's OpenAPI definition, effectively disabling global security. Example: @SecurityRequirements // Empty - overrides global security with [] @PostMapping("/public-endpoint") public ResponseEntity<?> publicEndpoint { // No security required } This works in libraries like springdoc-openapi and swagger-core, as confirmed in multiple issues and Stack Overflow answers. For non-OAuth schemes, scopes are empty []. Recent OpenAPI spec clarifications (3.0.4+) affirm security: [] removes inherited security. Alternative: @SecurityRequirements(value = {}) explicitly sets empty value array. Note: @Operation(security = {}) may not always produce the empty array; direct @SecurityRequirements is more reliable for override.
Citations:
- 1: Multiple SecurityRequirements annotations with AND condition swagger-api/swagger-core#3556
- 2: https://stackoverflow.com/questions/59357205/springdoc-openapi-apply-default-global-securityscheme-possible
- 3: https://www.speakeasy.com/openapi/security
- 4: disable global security for particular operation swagger-api/swagger-core#2844
- 5: https://stackoverflow.com/questions/50640517/swagger-core-2-0-disable-security-for-endpoint
- 6: Clarification about the meaning of an empty security array OAI/OpenAPI-Specification#3938
- 7: https://swagger.io/docs/specification/v3_0/authentication
- 8: global SecurityRequirement definition cannot be removed for single operations springdoc/springdoc-openapi#1111
- 9: Disable security for one operation springdoc/springdoc-openapi#259
🏁 Script executed:
# Check other `@WorkspaceScopedController` endpoints to see if they also use empty `@SecurityRequirements`
rg -B5 "@WorkspaceScopedController" server/application-server/src/main/java --type java -A30 | grep -A20 "@GetMapping" | head -50Repository: ls1intum/Hephaestus
Length of output: 6924
🏁 Script executed:
# Look at the actual test to see what happens without authentication
tail -200 server/application-server/src/test/java/de/tum/in/www1/hephaestus/practices/finding/PracticeFindingControllerIntegrationTest.java | head -100Repository: ls1intum/Hephaestus
Length of output: 3517
🏁 Script executed:
# This is the smoking gun - check if the test actually validates 401 for unauthenticated
grep -B5 -A10 "returns 401 for unauthenticated" server/application-server/src/test/java/de/tum/in/www1/hephaestus/practices/finding/PracticeFindingControllerIntegrationTest.javaRepository: ls1intum/Hephaestus
Length of output: 2051
🏁 Script executed:
# Check if other `@WorkspaceScopedController` endpoints also have similar tests
rg "returns 401 for unauthenticated\|isUnauthorized" server/application-server/src/test --type java -B2 -A8 | head -50Repository: ls1intum/Hephaestus
Length of output: 45
🏁 Script executed:
# Check the WorkspaceScopedController implementation to understand the enforcement mechanism
find server -name "WorkspaceScopedController.java" -type f -exec cat -n {} \;Repository: ls1intum/Hephaestus
Length of output: 1340
🏁 Script executed:
# Find the WorkspaceScopedWebMvcConfiguration mentioned in the annotation
find server -name "WorkspaceScopedWebMvcConfiguration.java" -type f -exec cat -n {} \;Repository: ls1intum/Hephaestus
Length of output: 3451
🏁 Script executed:
# Check if there's a filter or interceptor that handles authentication
rg "WorkspaceContext|workspaceSlug" server/application-server/src/main/java -B3 -A3 --type java | grep -E "ArgumentResolver|HandlerInterceptor|Filter|Security" | head -30Repository: ls1intum/Hephaestus
Length of output: 5374
🏁 Script executed:
# Check if this empty `@SecurityRequirements` pattern is intentional across the codebase
# by looking for when it was introduced
git log -p --all -S "@SecurityRequirements" -- "*.java" | head -100Repository: ls1intum/Hephaestus
Length of output: 45
🏁 Script executed:
# Check if there's documentation about why this pattern is used
rg "SecurityRequirements.*empty\|SecurityRequirements.*override\|filter.*authentication\|WorkspaceContextFilter" server/application-server/src/main/java --type java -B3 -A3 | head -50Repository: ls1intum/Hephaestus
Length of output: 45
🏁 Script executed:
# Find and inspect the WorkspaceContextFilter
find server -name "*ContextFilter*.java" -o -name "*WorkspaceFilter*.java" | xargs cat -n 2>/dev/nullRepository: ls1intum/Hephaestus
Length of output: 35341
🏁 Script executed:
# Verify this pattern is intentional by checking if there are endpoints that properly declare security
rg "@SecurityRequirement\(name\s*=.*bearerAuth" server/application-server/src/main/java --type java | head -10Repository: ls1intum/Hephaestus
Length of output: 215
Remove @SecurityRequirements to inherit global authentication.
Lines 1879, 1910, 1935, and 1965 have security: [], which declares these endpoints as unauthenticated despite the WorkspaceContextFilter enforcing 401 for all unauthenticated requests at runtime. The empty @SecurityRequirements annotation in the controller deliberately overrides global bearerAuth, creating a spec-runtime mismatch that would generate incorrect client code. Delete the @SecurityRequirements annotation from each endpoint in the source controller so they inherit bearerAuth from @OpenAPIDefinition, then regenerate the OpenAPI spec per coding guidelines.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/application-server/openapi.yaml` at line 1879, Remove the empty
`@SecurityRequirements` annotations that are producing security: [] in the
generated OpenAPI (seen at lines producing security: []) so the endpoints
inherit the global bearerAuth; specifically open the controller containing the
annotated endpoints and delete the `@SecurityRequirements` annotation from each
affected handler method (the methods currently producing security: []), then
regenerate the OpenAPI spec so `@OpenAPIDefinition`'s bearerAuth is applied;
ensure no other method-level `@SecurityRequirements` remain and run the spec
generation to confirm security: bearerAuth is present.
| ContributorPracticeSummary: | ||
| type: object | ||
| description: Per-practice finding summary for a contributor | ||
| properties: | ||
| category: | ||
| type: string | ||
| description: Practice category | ||
| lastFindingAt: | ||
| type: string | ||
| format: date-time | ||
| description: Timestamp of most recent finding | ||
| negativeCount: | ||
| type: integer | ||
| format: int64 | ||
| description: Number of NEGATIVE findings | ||
| positiveCount: | ||
| type: integer | ||
| format: int64 | ||
| description: Number of POSITIVE findings | ||
| practiceName: | ||
| type: string | ||
| description: Practice name | ||
| practiceSlug: | ||
| type: string | ||
| description: Practice slug | ||
| totalFindings: | ||
| type: integer | ||
| format: int64 | ||
| description: Total number of findings | ||
| required: | ||
| - negativeCount | ||
| - positiveCount | ||
| - practiceName | ||
| - practiceSlug | ||
| - totalFindings |
There was a problem hiding this comment.
ContributorPracticeSummary is still missing the dashboard trajectory data.
The linked objective requires the summary endpoint to provide a recent verdict trajectory for each practice, but this schema only exposes aggregate counts and lastFindingAt. The dashboard cannot render the promised trend data from this contract. Add the trajectory field in the source DTO/projection and regenerate the OpenAPI/client artifacts. As per coding guidelines, "Never hand-edit generated artifacts in server/application-server/openapi.yaml, webapp/src/api/**/*, server/application-server/src/main/java/de/tum/in/www1/hephaestus/intelligenceservice/**, server/intelligence-service/openapi.yaml, server/intelligence-service/src/shared/db/schema.ts, and docs/contributor/erd/schema.mmd; regenerate instead using the appropriate generation commands".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/application-server/openapi.yaml` around lines 3333 - 3367, The
ContributorPracticeSummary schema is missing the recent verdict trajectory
required by the summary endpoint: update the source DTO/projection that
populates ContributorPracticeSummary (add a trajectory field containing the
recent verdict/time series data), update any mapping code that builds
ContributorPracticeSummary instances, then run the project’s OpenAPI/client
generation commands to regenerate server/application-server/openapi.yaml and all
derived client and server artifacts rather than hand-editing the generated file;
ensure the new trajectory property is included in the generated
ContributorPracticeSummary contract and returned by the summary endpoint.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/application-server/openapi.yaml (1)
3984-3985: Theevidencefield lacks a type, producingunknownin generated clients.The
evidenceproperty has a description but notype. Per OpenAPI 3.1, this allows any value. While the PR summary notes this is intentional ("evidence is typed as Object for clean OpenAPI schemas"), generated TypeScript clients will infer this asunknown, requiring consumers to cast or validate at runtime. Consider whether an explicittype: objectoradditionalProperties: truewould provide slightly better client ergonomics while preserving flexibility.This is a generated file, so any change would need to be made in the source DTO's
@Schemaannotation. As per coding guidelines, "Never hand-edit generated artifacts [...]; regenerate instead using the appropriate generation commands".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/openapi.yaml` around lines 3984 - 3985, The OpenAPI schema's evidence property is missing a type so generated clients get unknown; update the source DTO's `@Schema` annotation (not the generated openapi.yaml) to include an explicit type and allow additional properties (e.g., `@Schema`(type = "object", additionalProperties = SchemaAdditionalProperties.TRUE) or equivalent in your codebase) so the generated openapi.yaml will emit evidence: { type: object, additionalProperties: true } and TypeScript clients will infer a usable object type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/application-server/openapi.yaml`:
- Around line 3984-3985: The OpenAPI schema's evidence property is missing a
type so generated clients get unknown; update the source DTO's `@Schema`
annotation (not the generated openapi.yaml) to include an explicit type and
allow additional properties (e.g., `@Schema`(type = "object", additionalProperties
= SchemaAdditionalProperties.TRUE) or equivalent in your codebase) so the
generated openapi.yaml will emit evidence: { type: object, additionalProperties:
true } and TypeScript clients will infer a usable object type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91961503-50be-4cad-a85f-4aa696ee0f08
📒 Files selected for processing (1)
server/application-server/openapi.yaml
📚 Documentation Preview
|
|
🎉 This PR is included in version 0.49.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Description
Add 4 read-only workspace-scoped REST endpoints to expose practice detection findings to the contributor dashboard (#897). The
PracticeFindingRepositorypreviously only had write methods (insertIfAbsent,deleteAllByPracticeWorkspaceId) — this PR adds the full read layer: controller, service, DTOs, repository queries, and integration tests.Closes #896
Endpoints
GET/workspaces/{slug}/practices/findingspracticeSlug,verdictGET/workspaces/{slug}/practices/findings/summaryGET/workspaces/{slug}/practices/findings/{findingId}GET/workspaces/{slug}/practices/findings/pull-request/{prId}Architecture decisions
@WorkspaceScopedController— follows existingAgentJobController/PracticeCatalogControllerpattern, auto-prefixes routes with/workspaces/{workspaceSlug}findByIdAndContributorAndWorkspacepushes the contributor ownership check into the JPQL query. This avoids lazy-load fragility ongetContributor()and makes the auth check atomic with the fetch. Non-owners get 404 (not 403) to avoid leaking finding existence.getCurrentUser()— all service methods useOptional<User>fromUserRepository.getCurrentUser(). List/summary return empty for unsyncced users. Detail returns 404.Object, notJsonNode— matchesAgentJobDTOpattern, produces clean OpenAPI schema (no broken$ref)@NonNull Longfor aggregate counts — projection and DTO use boxedLongwith@NonNullto satisfy annotation conventions and producerequiredfields in OpenAPI contractcountQuery— paginated query usesJOIN FETCHwhich is incompatible with Hibernate's count projections, so we provide an explicitcountQueryper the existing codebase patternFiles changed
PracticeFindingController.javaPracticeFindingService.java@Transactional(readOnly=true)PracticeFindingRepository.javadto/PracticeFindingListDTO.javadto/PracticeFindingDetailDTO.javadto/ContributorPracticeSummaryDTO.javadto/ContributorPracticeSummaryProjection.javaActivityModuleBoundaryTest.javaPracticeFindingControllerin arch rulesPracticeFindingControllerIntegrationTest.javaopenapi.yamlwebapp/src/api/*Security
WorkspaceScopedControllerfilteragentJobId,idempotencyKey, rawcontributorIdomitted from all DTOsTest coverage (28 tests)
List endpoint (12 tests): empty page, own-only filtering, practice slug filter, verdict filter, combined filters, pagination, page size cap, boundary normalization (
page=-1,size=0), full shape assertion, no internal field leaks, sort order, workspace isolationSummary endpoint (4 tests): empty list, aggregation with all fields, 401 unauthenticated, user isolation
Detail endpoint (6 tests): full detail with all fields, other user's finding → 404, non-existent → 404, 401 unauthenticated, evidence JSON round-trip, cross-workspace → 404
PR endpoint (6 tests): PR findings, includes other users, empty for unknown PR, sort order, 401 unauthenticated, workspace isolation
How to test
cd server/application-server && mvn test -Dtest="PracticeFindingControllerIntegrationTest" -Dsurefire.includedGroups="integration" -DskipTests=false -Dmaven.test.skip=falsemvn test -Dsurefire.includedGroups="architecture" -DskipTests=false -Dmaven.test.skip=falseGET /workspaces/{slug}/practices/findingswith valid JWT — verify own findings only, no internal fieldsSummary by CodeRabbit