General: Align compliance rules and stabilize generation workflow - #2631
General: Align compliance rules and stabilize generation workflow#2631ge94zec wants to merge 14 commits into
General: Align compliance rules and stabilize generation workflow#2631Conversation
- load compliance rules via aiService and inject dynamically into analyze/generateDraft
github-actions
Bot
added
server
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| UnusedCode | 1 medium |
| ErrorProne | 1 medium |
| Security | 1 high |
| Complexity | 1 medium |
🟢 Metrics 32 complexity
Metric Results Complexity 32
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
📊 Client Test Coverage Too Low 🔍 View coverage locally: pnpm run test:ci
open build/test-results/vitest/coverage/index.html🌐 View coverage from GitHub: |
ge94zec
changed the title
ge94zec
changed the title
General: Modularize compliance rules
|
📊 Client Test Coverage Too Low 🔍 View coverage locally: pnpm run test:ci
open build/test-results/vitest/coverage/index.html🌐 View coverage from GitHub: |
|
🤖 No OpenAPI or client changes needed. |
ge94zec
changed the title
General: Modularize compliance rules General: Update compliance rules in generation
…-rules-across-prompts' into chore/2535-modularize-compliance-rules-across-prompts
|
🤖 No OpenAPI or client changes needed. |
- add AiRun to centralize AbortController ownership and stale workflow detection - cancel active analysis and translation requests before starting generation -> prioritize generation - scope backend AI cancellation by job ID to avoid affecting other jobs or users - pass the job ID through generation and translation requests - return analysis score and compliance issues directly through JobAnalysisDTO - prevent persistence entities from leaking through the analysis API - add a dedicated streaming editor update method - remove redundant frontend score polling and additional job reloads - remove redundant analyze rules in AnalyzeComplianceText.st - add tests for cancellation, streaming, and editor behavior
|
📊 Client Test Coverage Too Low 🔍 View coverage locally: pnpm run test:ci
open build/test-results/vitest/coverage/index.html🌐 View coverage from GitHub: |
|
🤖 OpenAPI spec and client code auto-updated and committed. |
|
🤖 No OpenAPI or client changes needed. |
ge94zec
changed the title
General: Update compliance rules in generationGeneral: Align compliance rules and stabilize generation workflo
ge94zec
changed the title
General: Align compliance rules and stabilize generation workfloGeneral: Align compliance rules and stabilize generation workflow
…-rules-across-prompts' into chore/2535-modularize-compliance-rules-across-prompts
|
🤖 No OpenAPI or client changes needed. |
|
🤖 No OpenAPI or client changes needed. |
- add tests
|
🤖 No OpenAPI or client changes needed. |
az108
left a comment
az108
left a comment
There was a problem hiding this comment.
Requesting changes — the branch does not merge into main, and that needs sorting before the rest is worth acting on.
Blocking: 14 merge conflicts, three of them add/add
Last merge from main was 08-14. git merge-tree origin/main HEAD exits 1:
CONFLICT (add/add): ai/dto/ComplianceIssueDTO.java <- now exists on main
CONFLICT (add/add): ai/dto/JobAnalysisDTO.java <- now exists on main
CONFLICT (add/add): generated/model/job-analysis-dto.ts
CONFLICT (content): AiService.java, AiResource.java, JobService.java,
TranslateComplianceDTO.java, openapi.yaml,
job-creation-form.component.ts, editor.component.ts,
ai-resource-api.ts, translate-compliance-dto.ts,
AiResourceTest.java, job-creation-form.component.spec.ts
ComplianceIssueDTO and JobAnalysisDTO were created independently on main (via #2470/#2465) and here — two implementations of the same DTOs that need reconciling rather than textual merging.
Worth being deliberate about the resolution: add/add and modify/delete are exactly the conflict class that left two 0-byte files committed on #2520, so it is worth diffing against main deliberately rather than accepting either side wholesale.
I would rebase before acting on the inline point below — all three AiPriorityService call sites live in AiService.java, which is itself conflicted.
AiPriorityService has no test coverage
No test anywhere in src/test/ references it. AiResourceTest structurally cannot cover it: setUp() swaps in a Mockito mock via ReflectionTestUtils.setField(aiResource, "aiService", aiService), so neither the real AiService nor AiPriorityService ever runs. The new shouldReturnConflictWhenAnalysisIsCancelled stubs the CancellationException itself and only asserts the exception-to-409 mapping in AiResource.
That leaves untested: registration/deregistration, whether foreground actually cancels a live background, the unregister cleanup path, and the null-jobId bypass — i.e. all of the concurrency semantics, including the race below. A StepVerifier test over foreground/background would cover it.
Fine as-is
ai-run.spec.ts is good — both names start with should, and it covers both states of the run. The AiRun abstraction itself reads well.
| } | ||
| return Flux.defer(() -> { | ||
| Sinks.Empty<Void> cancellation = Sinks.empty(); | ||
| backgroundCancellations.computeIfAbsent(jobId, _ -> ConcurrentHashMap.newKeySet()).add(cancellation); |
There was a problem hiding this comment.
Check-then-act: computeIfAbsent returns the set, then .add(cancellation) runs outside any map lock — while cancelBackground (line 60) does backgroundCancellations.remove(jobId). If the remove lands between the two, the new cancellation goes into a set that is no longer in the map, so that background stream is never cancelled — which is the whole purpose of this class.
Being straight about severity: the window is a few bytecode instructions, so a hit is unlikely. Worth fixing anyway because the fix is a risk-free one-liner and the consequence is not cosmetic — an orphaned cancellation means a full background LLM call runs alongside the generation, and in the compliance path jobService.updateAiAnalysis (AiService.java:523) then writes a score computed from the pre-generation text.
compute holds the bin lock across the entire mapping function, so it is mutually exclusive with remove on the same key:
backgroundCancellations.compute(jobId, (_, existing) -> {
Set<Sinks.Empty<Void>> set = existing != null ? existing : ConcurrentHashMap.newKeySet();
set.add(cancellation);
return set;
});No memory leak either way — unregister uses computeIfPresent, so in the race case it degrades to a safe no-op and the orphaned set is collected.
Separate, lower-severity point on the same class: cancelBackground runs exactly once, at foreground subscribe, and leaves no marker behind. Generation streams for seconds, so background work registered after that point runs unimpeded. The client closes the obvious path — executeAutoSave bails on isGeneratingDraft() (job-creation-form.component.ts:1662) and re-checks run.isStale() after the save round-trip — but two paths remain, both with millisecond rather than nanosecond windows: two tabs on the same draft (no shared activeAiRun, which is presumably why this state is keyed by jobId server-side), and latency skew where generate is sent later but arrives first. Note the compute fix above does not close this one. A foreground marker that background checks at registration would.
2 participants
Checklist
General
Server
Motivation and Context
Translation, compliance analysis, and generation requests could overlap and finish out of order. A stale analysis or translation could then update the UI or the persisted job state after a newer generation had already completed, most visibly in the compliance score, which could briefly show the value belonging to an olderjob description.
The root cause on the server was that compliance analysis used a blocking
.call(), which could not be cancelled once started; on the client, nothing invalidated callbacks belonging to superseded runs.Description
Overall, these changes make the Generate → Translate → Analyze → Score workflow more deterministic. New generations take priority over outdated background work, stale results are discarded, and the displayed compliance score now corresponds to the latest analyzed version of the job description.
AiPriorityServicewithforeground(jobId, flux)/background(jobId, flux),scoped per job. Generation is foreground; translation and compliance analysis are
background. Starting a generation cancels ongoing background work for the same job.
.call()to.stream()so it can actually becancelled mid-flight; the response is reassembled and parsed via
BeanOutputConverter.AbortControllerfor generation,takeUntilfor the analysis request.POST /ai/jobs/analyzenow returnsJobAnalysisDTO(score + issues) instead ofComplianceIssue[]. Regenerate the API client.translateTextStreamtakes an additionaljobId.Steps for Testing
Prerequisites:
Review Progress
Code Review
Manual Tests
Screenshots
Test Coverage
Client
Server
Last updated: 2026-08-19 16:23:52 UTC