feat(server): worker runtime substrate + WSS control channel - #1302
Merged
FelixTJDietrich merged 32 commits intoMay 23, 2026
Conversation
Two focused unit tests on the new `prepareAndExecute` branch:
- `overridesPracticeRequestWhenWorkerLlmConfigured`: when the worker pod
has `hephaestus.worker.llm.{base-url,api-key}` set, the captured
`PracticeAgentRequest` must carry `credentialMode=API_KEY`, the
operator's apiKey, and the operator's baseUrl — confirming the BYO
worker path bypasses the app-pod LLM proxy.
- `leavesSnapshotCredentialModeWhenWorkerLlmUnset`: when the worker LLM
is unset, the request preserves the snapshot's `PROXY` mode and null
baseUrl — guarding against accidental override-without-config.
Uses Mockito `ArgumentCaptor<PracticeAgentRequest>` against the existing
`setupFullExecution()` harness; the snapshot is `PROXY/ANTHROPIC` so the
override is the only branch that flips the credential mode.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Three issues uncovered while standing up the worker substrate live against
the GPU ASE LLM gateway. Each one blocked the worker pod from booting in a
realistic role split (`HEPHAESTUS_RUNTIME_SERVER_ENABLED=false`).
1. **`AgentJobExecutor` gated only on `agent.nats.enabled`** — the bean
wires `SandboxManager`, but the sandbox is gated on the worker role.
App-pods (server-only) crashed with `No qualifying bean of type
SandboxManager`. Added the same `@ConditionalOnProperty(WORKER_PROPERTY,
matchIfMissing=true)` that `AgentNatsConsumerConfig` already carries.
2. **OAuth2 resource server intercepts worker JWTs at the upgrade path** —
`/api/workers/connect` carries the worker's own JWT (signed by
`WorkerKeyRing`), which is foreign to the user-facing OAuth2 chain. The
`BearerTokenAuthenticationFilter` runs before the `permitAll()` matcher
and triggers a Keycloak round-trip that fails (issuer unreachable in
worker pods). Added a dedicated `@Order(HIGHEST_PRECEDENCE)` filter
chain via `securityMatcher("/api/workers/**", "/actuator/**", "/gitlab",
"/github")` that skips OAuth2 entirely. The user-facing chain becomes
conditional on `JwtDecoder` so worker-only pods boot without any
Keycloak realm; a fallback `deny-all` chain locks down all other paths.
3. **`WebhookConfiguration` ambiguous `Connection` injection** — adding
the agent NATS connection (separate stream) made `Connection` ambiguous.
Three `@Bean` methods needed `@Qualifier("natsConnection")` to bind to
the sync connection. Spring's parameter-name resolution requires the
`-parameters` compiler flag which the build doesn't set.
These three fixes are what made the worker-pod live boot in this PR's
testing actually work — capacity reports flow, drain on SIGTERM is clean,
sessions show in `worker.hub.sessions.active`.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Loop-1 security + reliability fixes informed by a principal-engineer review.
Worst-link items dragged the hub to C+; this lifts every below-4 item.
JWT discipline (RFC 7519 / RFC 8725 §3.1):
- Add `audience` to WorkerTokenProperties (default `hephaestus-worker`);
issuer + verifier both bind it. Tokens forged for a different surface that
happens to share `iss` will no longer pass.
- Issuer sets `nbf` so a slightly future token can't ride a permissive
verifier into early validity.
- Verifier explicitly allowlists `RS256` on decoded.getAlgorithm() before
the kid dispatch, guarding against alg-confusion if a future refactor
changes the verifier shape.
- Verifier no longer holds the private key — `Algorithm.RSA256(pub, null)`.
WS upgrade hardening:
- HubWebSocketRegistration drops `setAllowedOrigins("*")`. Non-browser
workers don't send Origin and pass through; browsers can't construct
Bearer-authenticated upgrades and have no business reaching here.
Closes CSWSH (PortSwigger / OWASP WS cheat sheet).
Backpressure:
- HubProperties adds `sendBufferSizeBytes` (8 MiB default) and
`sendTimeLimit` (10s default). Handler wraps the raw transport with
ConcurrentWebSocketSessionDecorator so a slow worker can't balloon hub
memory or pin a sender thread.
- Refuse binary frames with code 1003 (text-only protocol) — close before
Jackson sees anything we'd have to defensively reason about.
ForceReconnect spam:
- Per-session AtomicBoolean once-flag stops the hub from re-emitting
ForceReconnect on every inbound frame during the rotation window.
- 60 redundant frames per worker → exactly 1.
Denylist cache stampede:
- `cache.get(jti, repository::existsById)` single-flight load. Ten parallel
WSS upgrades for the same jti collapse to one Postgres query.
Graceful shutdown:
- WorkerSessionRegistry implements SmartLifecycle at
`WebServerGracefulShutdownLifecycle.SMART_LIFECYCLE_PHASE - 1` so worker
sessions get a clean ForceReconnect + close-GOING_AWAY before Tomcat
stops accepting traffic.
Brute-force throttle on exchange:
- Per-source-IP Caffeine failure counter with 10 fails/min ceiling. The
registration token is high-entropy so exhaustive search is impractical,
but the counter caps log noise and makes alerting tractable.
YAGNI deletions:
- `FrameEnvelope.traceparent` — reserved field for "future OTel epic" with
no producer. Ship it when there's a consumer.
- `SessionKind.PRACTICE_REVIEW` — reserved enum constant for "future BYO".
Same logic.
Hub dispatch + close helper cleanup:
- Inbound switch arms for worker-source frames collapsed onto a single
warnUnexpectedFrame helper (kills 12 lines of one-line repetition).
- One-line close(WorkerSession, CloseStatus) passthrough inlined.
- Stale "// Inbound timestamp already marked" deleted.
Comment hygiene:
- WorkerTokenProperties + JavaJwtWorkerJwtVerifier + WorkerJwtIssuer +
HubWebSocketRegistration + WorkerSession lost javadocs that restated the
type signature or the `@ConditionalOnProperty` line above them.
Tests:
- WorkerJwtTest constructor sites updated for the new audience field.
- All 47 worker-substrate unit tests still green.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Three principal-engineer audits applied with worst-link grading; no component finished below A− and the ADR went from F (bloat) to A (Nygard compliant). ADR 0009 (worst-link before): - 2491 words → 619 words (-75%). - Killed "Shipped in this PR", "Tests" enumeration, "Out of scope", "Operational bootstrap" + "End-to-end playbook" shell scripts, and the zero-config boot transcript. PR-description content does not belong in a permanent decision record (Nygard 2011). - Compressed each multi-paragraph sub-decision to one row in a table. Comment hygiene across worker substrate: - WorkerProperties: dropped 13-line javadoc restating record fields. - WorkerControlClient: replaced 19-line threading-model javadoc with one paragraph; killed "// dispatcher accessed via" comment that restated the field name. - MentorSessionRunner: stripped the 4-line WHY block above the per-session lock down to one line; removed the defensive try/catch on Disposable.dispose() (the Reactor contract IS idempotent — defensive guards on framework contracts are theatre). - package-info.java × 3 (worker, worker.protocol, hub): each compressed to ≤ 4 lines covering only the modulith boundary and the protocol invariant. - WorkerJwtIssuer + JavaJwtWorkerJwtVerifier + WorkerTokenProperties + HubWebSocketRegistration + WorkerSession: removed multi-line javadocs that restated the type signature one line below. - WorkerSessionRegistry: 9-line class javadoc compressed to 4 lines. - MentorSessionBridge: removed `(#1100)` inline issue reference (PR numbers rot — the decision lives in ADR 0009). - HubWebSocketRegistration / WorkerControlWebSocketHandler / hub package-info: removed ADR-back-references that didn't carry new information. YAML / compose / migration: - application-worker.yml halved: dropped the `nats.server: nats://disabled` dummy workaround entirely (see NatsProperties fix below). - compose.app.yaml worker block: stripped paraphrased ADR refs, orphan "Hostname is …" comment with no env var, and copy-pasted ADR-section pointers; comments now only mark non-obvious WHYs. - Both new Liquibase changesets: 4-line `<comment>` block → one sentence. NatsProperties dummy-server footgun killed at the validator level: - `@NotBlank` on `server` fired even when `enabled=false`, forcing the worker overlay to ship a sentinel `nats://disabled:4222` and a three-line apologetic comment. Moved validation into the compact constructor: only required when enabled. Now: zero workarounds. Test dedup: - New `core/runtime/worker/testing/` package: `CapturingPublisher` + `WorkerPropertiesFixtures.minimal(...)` / `.withDrain(...)`. - MentorSessionRunnerTest, WorkerCapacityStateTest, WorkerCapacityReporter- Test, WorkerDrainCoordinatorTest: 3× `CapturingPublisher` copy + 4× WorkerProperties 7-arg builder collapsed to the shared fixtures. - AgentJobExecutorTest: dropped 3 narration comments (`// Build a real AgentJob instead of a mock`, `// For the complete phase`, `// Bug fix: only transition from RUNNING, not QUEUED`) — code is self-evident or the rationale lives in the commit message where it belongs. Tests: 47 worker-substrate unit + 4 architecture (RuntimeRoleBoundaryTest) still green. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
A brutal Loop-2 re-grade against the Loop-1 state found two real
security regressions and one inverted lifecycle phase. All fixed here
plus the higher-priority gaps the audit listed.
CRITICAL: actuator scope regression
- workerHubSecurityFilterChain matched `/actuator/**` permitAll, broadening
the public actuator footprint from `/actuator/health{,/**}` + `/info`
(pre-PR baseline) to every actuator endpoint Spring exposes. Metrics
and prometheus are now anonymous low-grade exfil channels on app pods.
- Narrowed securityMatcher to the original three actuator paths only;
everything else falls through to the OAuth2 chain.
CRITICAL: bridge user-controlled image/command/workspaceId default-on
- BridgedMentorController accepts a raw JsonNode `context` and the worker
uses `context.image()`, `context.command()`, `context.environment()`,
`context.userId()`, `context.workspaceId()` to build the sandbox spec.
Combined with `matchIfMissing=true` on the SERVER_PROPERTY gate, every
monolith/dev pod loaded this controller and the bridge bean.
- Gated both the controller and the bridge bean on a new
`hephaestus.worker.hub.bridge.enabled` property defaulting to false.
The full server-side context construction (allowlist image, verify
workspace membership, derive userId from auth principal) is a separate
follow-up; for this PR the surface is opt-in only.
MAJOR: SmartLifecycle phase math inverted
- WorkerSessionRegistry.getPhase() returned `SMART_LIFECYCLE_PHASE - 1`,
which means it stops AFTER the embedded web server, not before — phases
stop in DESCENDING order. The commit message + in-code comment both
asserted the opposite.
- Flipped to `+ 1`; updated comment to match.
MAJOR: WorkerHello timeout — half-open sessions could leak forever
- A client that completes the WSS upgrade with a valid JWT but never
sends WorkerHello sat in the handler attributes forever, tying up the
connection for the JWT's full lifetime.
- Added `hephaestus.worker.hub.helloTimeout` (default 10s) bound on
HubProperties. afterConnectionEstablished schedules a close-with-1011
on the timeout; handleHello cancels it. Stored on WorkerSession so
cancellation is straightforward.
MAJOR: WorkerControlClient silence-deadline cold-start bug
- `lastInboundAt` starts at Instant.EPOCH. On the first iteration of the
connect-loop after a successful open, Duration.between(EPOCH, now)
exceeds any reasonable silence threshold → spurious immediate
reconnect.
- Prime lastInboundAt on successful openWebSocket so the silence check
only fires when actual silence happens after a healthy connect.
MINOR: health-indicator threshold mismatch
- WorkerControlChannelHealthIndicator used 2× heartbeat; client uses 3×.
Harmonized to 3× so health flips DOWN at the same threshold the client
decides to reconnect.
MINOR: defensive null-check on Spring-injected param
- WorkerKeyRing.fromConfig had `Objects.requireNonNull(properties)`.
Spring guarantees non-null bean injection. Deleted.
Audit metrics (Rubric B item 9 — was missing):
- worker.jwt.verify{outcome=success|failed, reason=alg|kid|sig|claim|revoked|decode|missing}
- worker.token.exchange{outcome=success|failed, reason=disabled|throttled|bad-payload|bad-token}
- worker.hub.hello.timeout counter
- WorkerJwtInvalidException now carries a `reasonTag` for clean reason
cardinality on the failure counter.
Tests: 47 worker-substrate unit tests still green. WorkerJwtTest updated
for the new MeterRegistry constructor arg on the verifier.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Found by a Loop-2 impact-analysis pass: the worker-pod compose block looked
ready-to-deploy but would have failed in production for two distinct
reasons. Both fixed.
1. KEYCLOAK_URL placeholder crashes the worker pod under prod,worker:
`application-prod.yml` defines
spring.security.oauth2.resourceserver.jwt.issuer-uri:
${KEYCLOAK_URL}/realms/${KEYCLOAK_REALM}
with no fallback. The worker compose block doesn't set those envs and
shouldn't — the worker pod has no user-facing API and no need for the
OAuth2 resource server. Spring placeholder resolution fails at boot
before any role gating runs.
Fix: `application-worker.yml` excludes
`OAuth2ResourceServerAutoConfiguration` so the issuer-uri property is
never resolved. SecurityConfig already gates the user-facing chain on
`@ConditionalOnBean(JwtDecoder.class)`; with no JwtDecoder, the
lockdown chain (deny-all) covers any non-worker-hub path.
2. Empty HEPHAESTUS_HUB_URL wired the WSS client anyway:
`application-worker.yml` defaults the endpoint to `${HEPHAESTUS_HUB_URL:}`
(empty string). `@ConditionalOnProperty(name="endpoint")` treats any
non-null value as a match — empty string included — so the
`WorkerControlClient` bean was constructed even when no hub URL was set.
The connection loop's `isConfigured()` guard saved it from actually
dialing, but the bean wiring still happened.
Fix: `@ConditionalOnExpression("'${...endpoint:}'.length() > 0")` so
the client bean isn't even instantiated when the URL is empty. The
logging publisher fallback then wins via `@ConditionalOnMissingBean`.
Tests: 47 worker-substrate unit tests still green.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Run by the pre-push husky hook on first push attempt. No behavioral changes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements a WSS worker hub and worker runtime: protocol frames, FrameCodec, WebSocket handler/registry, worker control client, JWT exchange + denylist, capacity/drain lifecycle, mentor SSE bridge, Compose/profile and DB migrations, practice criteria API/schema change, and tests. ChangesWorker hub + runtime
Sequence Diagram(s)sequenceDiagram
participant Worker as WorkerControlClient
participant Exchange as /api/workers/exchange
participant Hub as WorkerControlWebSocketHandler
participant Registry as WorkerSessionRegistry
participant Bridge as MentorSessionBridge
participant DB as Postgres (denylist)
Worker->>Exchange: POST registration token -> token
Worker->>Hub: WebSocket connect (Authorization: Bearer token)
Hub->>Hub: WorkerJwtHandshakeInterceptor verifies token
Hub->>Registry: register(WorkerSession)
Worker->>Hub: WorkerHello frame
Hub-->>Worker: WorkerWelcome
Worker->>Hub: CapacityReport (heartbeat)
Hub->>Registry: update session capacity
Bridge->>Hub: open mentor session -> SessionOpen
Hub->>Worker: SessionOpen forwarded
DB-->>Hub: denylist checks on verify / revoke
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
📚 Documentation PreviewThis PR includes documentation changes. A preview has been deployed: Preview for commit 5d2c36d. Updates automatically on new commits. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (17)
server/src/main/java/de/tum/cit/aet/hephaestus/config/KeycloakConfig.java (1)
7-8: ⚡ Quick winUse Lombok
@Slf4jinstead of manualLoggerFactorywiring.Please align this class with the repo logging standard and replace the manual logger field with
@Slf4j.As per coding guidelines: "Use Lombok annotations:
@RequiredArgsConstructor,@Slf4j,@Getter,@Setter,@Builder; avoid@Data".Also applies to: 16-16
🤖 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/hephaestus/config/KeycloakConfig.java` around lines 7 - 8, Replace the manual SLF4J wiring in KeycloakConfig (currently importing org.slf4j.Logger and org.slf4j.LoggerFactory and declaring a private static final Logger) with Lombok's `@Slf4j`: remove the Logger/LoggerFactory imports and the manual logger field, add the lombok.extern.slf4j.Slf4j annotation to the KeycloakConfig class declaration, and use the generated log field (log) everywhere it is referenced; ensure Lombok is available in the project if not already.server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerTokenExchangeController.java (1)
27-55: ⚡ Quick winUse Lombok-based constructor injection and logging conventions in this controller.
Prefer
@RequiredArgsConstructor+@Slf4jover manual constructor andLoggerFactoryin Spring-managed classes.As per coding guidelines `server/**/*.java`: Use constructor injection via `@RequiredArgsConstructor` Lombok annotation for all Spring-managed bean dependencies; use Lombok annotations including `@Slf4j`.Suggested patch
+import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; ... +@Slf4j +@RequiredArgsConstructor public class WorkerTokenExchangeController { - - private static final Logger log = LoggerFactory.getLogger(WorkerTokenExchangeController.class); private static final int MAX_FAILURES_PER_IP_PER_MINUTE = 10; ... - public WorkerTokenExchangeController( - WorkerJwtIssuer issuer, - WorkerTokenProperties properties, - MeterRegistry meterRegistry - ) { - this.issuer = issuer; - this.properties = properties; - this.meterRegistry = meterRegistry; - }🤖 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/hephaestus/core/runtime/hub/auth/WorkerTokenExchangeController.java` around lines 27 - 55, Replace the manual Logger and explicit constructor in WorkerTokenExchangeController with Lombok annotations: annotate the class with `@RequiredArgsConstructor` and `@Slf4j`, remove the private static final Logger log and the explicit public WorkerTokenExchangeController(...) constructor, keep the final fields issuer, properties, meterRegistry (they will be injected via the generated constructor) and preserve the failuresByIp field initializer; add the necessary Lombok imports (lombok.RequiredArgsConstructor, lombok.extern.slf4j.Slf4j) and remove unused LoggerFactory import.server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/LoggingWorkerControlPublisher.java (1)
15-20: ⚡ Quick winApply the project logging convention in this fallback publisher.
On Line 15–Line 20, switch from
LoggerFactoryfield to Lombok@Slf4jfor consistency with the rest of the server codebase standard.As per coding guidelines
server/**/*.java:Use Lombok annotations:@requiredargsconstructor,@Slf4j``.🤖 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/hephaestus/core/runtime/worker/LoggingWorkerControlPublisher.java` around lines 15 - 20, Replace the manual LoggerFactory logger field in LoggingWorkerControlPublisher with Lombok's `@Slf4j`: remove the private static final Logger log = LoggerFactory.getLogger(...) declaration and add the `@Slf4j` annotation to the class so the existing send(WorkerControlFrame frame) method can use the generated "log" instance; also remove unused imports for org.slf4j.Logger and LoggerFactory if present to keep imports consistent with the project's Lombok logging convention.server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/session/BridgedMentorController.java (1)
44-50: ⚡ Quick winAlign controller wiring/logging with project Lombok conventions.
On Line 44–Line 50, replace manual
LoggerFactory+ explicit constructor with@Slf4jand@RequiredArgsConstructorto match the enforced Spring class pattern.As per coding guidelines
server/**/*.java:Use constructor injection via@requiredargsconstructorLombok annotation for all Spring-managed bean dependenciesandNever use System.out.println() for logging; use@slf4jannotation and SLF4J logging instead.🤖 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/hephaestus/core/runtime/hub/session/BridgedMentorController.java` around lines 44 - 50, Replace the manual logger and explicit constructor in BridgedMentorController with Lombok annotations: remove the static Logger log field and the public BridgedMentorController(MentorSessionBridge bridge) constructor, add class-level annotations `@Slf4j` and `@RequiredArgsConstructor`, keep the final MentorSessionBridge bridge field as-is for constructor injection, and add the necessary Lombok imports (lombok.RequiredArgsConstructor and lombok.extern.slf4j.Slf4j) while removing now-unused imports.server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/session/MentorSessionBridge.java (1)
39-53: ⚡ Quick winUse Lombok-based constructor + logger pattern here too.
On Line 39–Line 53, prefer
@Slf4jand@RequiredArgsConstructorinstead of manual logger/constructor to stay consistent with the enforcedserver/**/*.javastandard.As per coding guidelines
server/**/*.java:Use constructor injection via@requiredargsconstructorLombok annotation for all Spring-managed bean dependenciesandUse Lombok annotations:@requiredargsconstructor,@Slf4j``.🤖 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/hephaestus/core/runtime/hub/session/MentorSessionBridge.java` around lines 39 - 53, Replace the manual Logger declaration and explicit constructor in MentorSessionBridge with Lombok annotations: annotate the class with `@Slf4j` and `@RequiredArgsConstructor`, remove the private static final Logger log and the explicit constructor that takes WorkerSessionRegistry, HubSessionRegistry and MeterRegistry, and make the dependency fields (workerRegistry, sessionRegistry, sessionsOpened, noCapacity) final if not already; keep EMITTER_TIMEOUT as-is and ensure lombok imports are added so the class relies on constructor injection generated by `@RequiredArgsConstructor` and logging via `@Slf4j`.server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/WorkerCapacityReporter.java (1)
22-38: ⚡ Quick winConverge on Lombok constructor/logging style in worker reporter.
On Line 22–Line 38, prefer
@Slf4jand@RequiredArgsConstructorinstead of manual logger + explicit constructor for this Spring-managed component.As per coding guidelines
server/**/*.java:Use constructor injection via@requiredargsconstructorLombok annotation for all Spring-managed bean dependenciesandUse Lombok annotations:@requiredargsconstructor,@Slf4j``.🤖 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/hephaestus/core/runtime/worker/WorkerCapacityReporter.java` around lines 22 - 38, The class WorkerCapacityReporter currently defines an explicit logger field "log" and a manual constructor; replace these with Lombok annotations by removing the private static final Logger log and the explicit constructor, annotate the class with `@Slf4j` and `@RequiredArgsConstructor` so Spring uses constructor injection for final fields (state, publisher, interval, scheduler, sent, failed) and use the generated constructor and logger throughout; ensure any use of "log" continues to work (no name change) and keep non-final or mutable fields like "task" as-is.server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/session/mentor/MentorSessionRunner.java (1)
25-27: ⚡ Quick winAlign with project Lombok logging/constructor conventions.
Please switch from manual
LoggerFactoryand explicit constructor style to the repository-standard Lombok pattern.As per coding guidelines
Use Lombok annotations:@requiredargsconstructor,@slf4j,@Getter,@Setter,@builder; avoid@Data``.Also applies to: 53-59
🤖 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/hephaestus/core/runtime/worker/session/mentor/MentorSessionRunner.java` around lines 25 - 27, The class MentorSessionRunner currently uses manual LoggerFactory and an explicit constructor; replace these with Lombok annotations: add `@Slf4j` to provide the logger and `@RequiredArgsConstructor` to generate the constructor for final fields, remove the explicit Logger and explicit constructor, and remove unused imports (org.slf4j.Logger, org.slf4j.LoggerFactory). Also ensure other Lombok-relevant classes follow project convention (use `@Getter/`@Setter/@Builder where required and avoid `@Data`) and update imports to include lombok.RequiredArgsConstructor and lombok.extern.slf4j.Slf4j as needed.server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/session/WorkerSessionDispatcher.java (1)
11-18: ⚡ Quick winUse
@Slf4j/ Lombok style for logger + constructor consistency.This class uses manual
LoggerFactoryand explicit constructor wiring. Please align with the project’s Lombok convention.As per coding guidelines
Use Lombok annotations:@requiredargsconstructor,@slf4j,@Getter,@Setter,@builder; avoid@Data``.Also applies to: 21-23
🤖 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/hephaestus/core/runtime/worker/session/WorkerSessionDispatcher.java` around lines 11 - 18, Replace the manual SLF4J logger and explicit constructor wiring in WorkerSessionDispatcher with Lombok annotations: remove the private static final Logger log = LoggerFactory... and annotate the class with `@Slf4j` and `@RequiredArgsConstructor` (and other required Lombok annotations per guideline if applicable); ensure any explicit constructor is removed so Lombok generates it and update usages of "log" to use the Lombok-provided logger; apply the same change pattern to the other classes referenced (lines 21-23) to keep constructor and logger style consistent.server/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/sync/NatsProperties.java (1)
61-61: 💤 Low valueValidation timing changed from Boot property binding to constructor.
The validation moved from
@NotBlank(enforced during Spring Boot property binding, producing aConstraintViolationException) to an explicitIllegalStateExceptionin the compact constructor. This changes the failure mode from collected validation errors (potentially multiple properties at once) to fail-fast single-property checks. Operators will see a different exception type at startup, but the guard logic is correct:isBlank()coversnull, empty, and whitespace-only strings.Also applies to: 67-69
🤖 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/hephaestus/gitprovider/sync/NatsProperties.java` at line 61, The compact constructor in NatsProperties replaced `@NotBlank-based` bean validation with manual isBlank checks that throw IllegalStateException; revert to bean-validation by restoring `@NotBlank` on the affected constructor parameters (e.g., server, username, password) and remove the manual isBlank/IllegalStateException guards from the NatsProperties compact constructor so Spring Boot/Validator can produce ConstraintViolationException during property binding; ensure the class is still configured for validation (e.g., annotated with `@Validated/`@ConfigurationProperties as previously).server/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerTokenExchangeIntegrationTest.java (1)
30-30: ⚡ Quick winUse
should...When...naming for this test method.Please align the method name with the repository’s standard test naming convention.
As per coding guidelines "Use
should[ExpectedBehavior]When[Condition]naming pattern for test methods".🤖 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/hephaestus/core/runtime/hub/auth/WorkerTokenExchangeIntegrationTest.java` at line 30, Rename the test method validRegistrationTokenReturnsVerifiableJwt to follow the should[ExpectedBehavior]When[Condition] pattern (for example shouldReturnVerifiableJwtWhenRegistrationTokenIsValid) and update any references to that method in the class; adjust the method name in WorkerTokenExchangeIntegrationTest (the test method itself) so annotations and imports remain intact.server/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/WorkerControlChannelIntegrationTest.java (1)
73-73: ⚡ Quick winUse
should[ExpectedBehavior]When[Condition]naming for test methods.These test names are descriptive but don’t follow the repository naming convention, which reduces consistency across test tiers.
As per coding guidelines "Use
should[ExpectedBehavior]When[Condition]naming pattern for test methods".Also applies to: 127-127
🤖 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/hephaestus/core/runtime/WorkerControlChannelIntegrationTest.java` at line 73, The test method name handshakeAndCapacityRoundTrip does not follow the repository's should[ExpectedBehavior]When[Condition] convention; rename it to a descriptive name following that pattern (for example shouldPerformHandshakeAndReturnCapacityWhenRoundTrip) and do the same for the other test mentioned at line 127, update any references/usages in the class (imports, annotations, test runner) and ensure the `@Test` methods compile and run under the new names so consistency with the test naming guideline is preserved.server/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/worker/WorkerCapacityReporterTest.java (1)
29-29: ⚡ Quick winRename tests to the
should...When...format.Adopting the shared naming pattern keeps unit tests consistent and easier to scan.
As per coding guidelines "Use
should[ExpectedBehavior]When[Condition]naming pattern for test methods".Also applies to: 47-47
🤖 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/hephaestus/core/runtime/worker/WorkerCapacityReporterTest.java` at line 29, Rename the test methods to follow the should[ExpectedBehavior]When[Condition] pattern: change emitsCapacityReportSnapshot() to something like shouldEmitCapacityReportSnapshotWhen[Condition] and likewise rename the other test at the referenced location to should[ExpectedBehavior]When[Condition]; update any references/imports or annotations that rely on the method names (e.g., the test runner), and ensure the new names are descriptive of the expected behavior and condition for the tests in WorkerCapacityReporterTest.server/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/worker/WorkerCapacityStateTest.java (1)
13-13: ⚡ Quick winApply
should[ExpectedBehavior]When[Condition]naming to these tests.Please align method names with the repository-wide convention for test readability and consistency.
As per coding guidelines "Use
should[ExpectedBehavior]When[Condition]naming pattern for test methods".Also applies to: 29-29, 38-38, 46-46
🤖 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/hephaestus/core/runtime/worker/WorkerCapacityStateTest.java` at line 13, Rename the test methods in WorkerCapacityStateTest to follow the repository convention should[ExpectedBehavior]When[Condition]; specifically rename reviewClaimReleaseSnapshotFlow (and the other three test methods in the same class) to descriptive names like shouldReviewClaimReleaseSnapshotFlowWhen[SpecificCondition] (replace [SpecificCondition] with the actual condition each test asserts), update the method identifiers (e.g., reviewClaimReleaseSnapshotFlow -> shouldReviewClaimReleaseSnapshotFlowWhenX) and any usages or annotations referencing them, and ensure method names remain valid Java identifiers and clearly express expected behavior and trigger condition.server/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/worker/WorkerControlChannelHealthIndicatorTest.java (1)
23-23: ⚡ Quick winRename test methods to the
should...When...convention.This keeps the health-indicator tests aligned with the project’s test naming standard.
As per coding guidelines "Use
should[ExpectedBehavior]When[Condition]naming pattern for test methods".Also applies to: 42-42
🤖 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/hephaestus/core/runtime/worker/WorkerControlChannelHealthIndicatorTest.java` at line 23, Rename the test method reflectsConnectionState (and its other test siblings in the same class) to follow the should[ExpectedBehavior]When[Condition] pattern; for example change reflectsConnectionState to shouldReflectConnectionStateWhenParametersProvided (or a concise variant like shouldReportHealthWhenConnectionStateChanges) and update any calls/annotations referencing reflectsConnectionState (e.g., display names or parameterized test references) so the test class compiles and the naming follows the project's should...When... convention.server/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerJwtTest.java (1)
55-55: ⚡ Quick winStandardize test names to
should[ExpectedBehavior]When[Condition].Current names are readable, but they diverge from the repository test naming convention.
As per coding guidelines "Use
should[ExpectedBehavior]When[Condition]naming pattern for test methods".Also applies to: 68-68, 78-78, 95-95, 110-110, 173-173, 212-212, 229-229
🤖 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/hephaestus/core/runtime/hub/auth/WorkerJwtTest.java` at line 55, Rename test methods in WorkerJwtTest to follow the repository convention should[ExpectedBehavior]When[Condition]: e.g. change issuedTokenVerifiesWithExpectedClaims to shouldVerifyIssuedTokenWhenExpectedClaimsPresent, and apply the same pattern to the other test methods referenced in the review (rename each existing method to start with "should", describe the expected behavior, then "When" plus the condition). Update any usages or annotations that reference the old method names (e.g., JUnit annotations remain the same) so the tests compile and run.server/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/hub/WorkerSessionRegistryTest.java (1)
26-26: ⚡ Quick winAlign test method names with
should...When...convention.Please rename these test methods to the repository’s standard naming pattern for consistency and discoverability.
As per coding guidelines "Use
should[ExpectedBehavior]When[Condition]naming pattern for test methods".Also applies to: 54-54
🤖 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/hephaestus/core/runtime/hub/WorkerSessionRegistryTest.java` at line 26, Rename test methods to follow the should[ExpectedBehavior]When[Condition] convention: change duplicateRegistrationEvictsOlderAndKeepsNewer to shouldEvictOlderAndKeepNewerWhenDuplicateRegistration and rename the other non-conforming test in WorkerSessionRegistryTest (the second test around the same block) to a matching should...When... form; ensure method names start with "should", describe the expected behavior then "When" plus the condition, and update any references/imports accordingly.server/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/worker/WorkerDrainCoordinatorTest.java (1)
35-35: ⚡ Quick winUse
should[ExpectedBehavior]When[Condition]names for drain coordinator tests.Renaming these methods would align the class with repository test naming conventions.
As per coding guidelines "Use
should[ExpectedBehavior]When[Condition]naming pattern for test methods".Also applies to: 70-70, 91-91, 113-113, 135-135
🤖 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/hephaestus/core/runtime/worker/WorkerDrainCoordinatorTest.java` at line 35, Rename the test methods to follow the repository convention "should[ExpectedBehavior]When[Condition]": e.g. change gracefulDrainAwaitsThenSucceeds to a name like shouldAwaitGracefulDrainWhenConditionsMet (or similar descriptive should...When... form), and rename the other test methods in the same class to the same pattern; update any references/imports or annotations that refer to the old method names so tests still run.
🤖 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/hephaestus/agent/job/AgentJobExecutor.java`:
- Around line 331-346: The current catch-all sets claimed = true even when
claimAndExecute failed (e.g., ClaimFailedException), causing releaseCapacity()
to run without a prior successful claim; update the exception handling in
AgentJobExecutor so that ClaimFailedException does not set claimed to true
(either add an explicit catch (ClaimFailedException e) that sets claimed = false
and handles the failure, or modify the generic catch to check for
ClaimFailedException before setting claimed = true), keeping existing handlers
handleCancellation and handleExecutionFailure for other errors and ensuring
releaseCapacity() is only called when a real claim succeeded.
- Around line 237-249: cancelInFlight currently queries all
AgentJobStatus.RUNNING jobs and cancels them globally; restrict cancellation to
this worker only by scoping the lookup or filter by the executor's worker id.
Update cancelInFlight to call a repository method that includes the worker id
(e.g., jobRepository.findByStatusAndWorkerId(AgentJobStatus.RUNNING,
thisWorkerId)) or filter the result of
jobRepository.findByStatus(AgentJobStatus.RUNNING) by job.getWorkerId(). Use the
executor's identifier (field or method on AgentJobExecutor) when selecting jobs
so transitionToCancelled is invoked only for jobs owned by this worker. Ensure
any new repository method signature matches existing repository patterns and
preserve the transactionTemplate.executeWithoutResult usage around
jobRepository.transitionToCancelled.
In `@server/src/main/java/de/tum/cit/aet/hephaestus/config/KeycloakConfig.java`:
- Around line 31-47: The current fallback uses
java.lang.reflect.Proxy.newProxyInstance which only works for interfaces but
org.keycloak.admin.client.Keycloak is a concrete class, causing runtime failure;
change the fallback to return an anonymous subclass (or a small concrete stub)
of Keycloak instead of a JDK dynamic proxy: implement/override close() to be a
benign no-op and override relevant public methods to throw IllegalStateException
with the existing message. Locate the branch guarded by
keycloakProperties.isConfigured() in KeycloakConfig (the code that currently
calls Proxy.newProxyInstance) and replace that proxy creation with
constructing/returning an anonymous Keycloak subclass that preserves the same
error behavior and message.
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerTokenDenylist.java`:
- Around line 42-47: The revoke method in WorkerTokenDenylist saves a
WorkerTokenDenylistEntry with the provided expiresAt without validation; add an
argument check at the start of revoke (in class WorkerTokenDenylist) to ensure
expiresAt is not null (and optionally not before Instant.now() if desired),
throwing IllegalArgumentException when invalid, before calling
repository.save(new WorkerTokenDenylistEntry(...)) and cache.put(jti,
Boolean.TRUE).
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerTokenExchangeController.java`:
- Around line 58-85: The exchange method in WorkerTokenExchangeController is
returning empty responses for error cases (disabled, throttled, bad-payload,
bad-token); update each error return to include an RFC-7807 ProblemDetail
payload (use ProblemDetail.of(...) or your app's helper via
`@RestControllerAdvice`) so clients get a consistent error body; specifically
replace the ResponseEntity.status(...).build() calls after
properties.isExchangeEnabled() check, the MAX_FAILURES_PER_IP_PER_MINUTE
throttle branch, the bad-payload branch (request null/blank), and the bad-token
branch (constantTimeEquals failure) with ResponseEntitys that contain a
ProblemDetail describing the error, status, and a machine-readable "reason"
matching the existing meterRegistry tag values (disabled, throttled,
bad-payload, bad-token) and keep the existing logging, failuresByIp increments,
meterRegistry counters, and AUDIT_METRIC tags intact.
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/session/BridgedMentorController.java`:
- Around line 54-55: Replace the generic authentication guard on the bridged
mentor endpoints with the mentor-only security annotation: in
BridgedMentorController, change the authorization on the
open(OpenSessionRequest) method and the corresponding input(...) and close(...)
methods to use the existing mentor-access utility (e.g. annotate with
`@RequireMentorAccess` or the project's mentor-access annotation) instead of
`@PreAuthorize`("isAuthenticated()"); locate the methods by name (open, input,
close) and update their annotations so only requests containing the
mentor_access JWT claim are permitted.
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/WorkerControlWebSocketHandler.java`:
- Around line 190-215: Move the premature session.cancelHelloDeadline() so it
only runs after the handshake fully succeeds: perform identity/version checks,
then wrap registry.register(session), session.send(welcome) and
meterRegistry.counter(...).increment() in a try/catch; on any exception
fail-closed by calling session.close(...) (e.g. CloseStatus.SERVER_ERROR or
appropriate CloseStatus) and do NOT cancel the hello deadline, otherwise cancel
the deadline after the successful send/metric increment. Apply the same change
pattern to the other handshake block that currently cancels the deadline earlier
(the other block around lines 152-160).
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/WorkerSessionRegistry.java`:
- Around line 45-52: The register method currently allows new WorkerSession
instances after shutdown/drain begins; update
WorkerSessionRegistry.register(WorkerSession incoming) to first check the
running flag and refuse new registrations when running is false (e.g., return a
clear failure value or throw an IllegalStateException) before calling
byWorkerId.compute; ensure the check is performed atomically with respect to the
registry lifecycle so late sessions cannot be inserted during or after the drain
path started (preserve existing eviction logic using the evicted AtomicReference
and compute only when running is true).
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/protocol/FrameCodec.java`:
- Line 24: The frame size checks in FrameCodec currently use json.length()
(character count) and must be changed to measure UTF-8 byte length: replace both
occurrences where json.length() is compared to MAX_FRAME_BYTES with
json.getBytes(StandardCharsets.UTF_8).length (or use CharsetEncoder) and import
java.nio.charset.StandardCharsets; update the conditional in the method(s)
inside class FrameCodec so the byte-length is validated against MAX_FRAME_BYTES
to avoid closing the WebSocket for multi-byte characters.
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/session/mentor/MentorSessionRunner.java`:
- Around line 155-176: After tryClaimMentor()/putIfAbsent you must ensure any
exception during context parsing, spec building, attach or subscription does not
leak the claimed session slot; update MentorSessionRunner so the block from
objectMapper.treeToValue(open.context(), MentorSessionContext.class) through
sandbox.subscribeFromNow(...) is wrapped to catch any
RuntimeException/InteractiveSandboxException (and other thrown errors), and on
any failure call failOpen(session) to release the session; if svc.attach(spec)
succeeded before a later failure, ensure the created sandbox is properly
released (e.g. call svc.detach(sandbox) or sandbox.close) before failing; also
only assign session.sandbox and session.subscription after successful
attach+subscribe so partial assignments are not left behind.
- Around line 227-230: The terminate method currently always calls teardown
which can double-tear down a session if another thread already removed/closed
it; change terminate(RunningSession session, SessionCloseReason reason) to only
call teardown when the session was actually removed from the sessions map (e.g.,
use sessions.remove(session.sessionId) and check the returned value is the same
instance or use sessions.remove(session.sessionId, session)); only invoke
teardown(session, reason) when the removal succeeded to avoid double-release and
duplicate terminal frames.
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/session/WorkerSessionDispatcher.java`:
- Around line 35-42: WorkerSessionDispatcher currently only logs when an
incoming SessionOpen (open) is unroutable (either open.kind() !=
SessionKind.MENTOR_INTERACTIVE or mentorRunner is empty) and never returns a
terminal frame; update the error paths to send an explicit SessionClose back to
the hub so the remote session is terminated. Concretely, in the
WorkerSessionDispatcher branch where mentorRunner.ifPresentOrElse() logs the
missing runner and in the else branch for unsupported kinds, construct and
dispatch a SessionClose (including session id from open, a clear reason string
and appropriate close code) via the same session/sender mechanism used elsewhere
in this class so the hub receives a terminal frame instead of leaving the
session pending. Ensure you use the existing MentorSessionRunner/onOpen flow
unchanged when present.
In
`@server/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerTokenExchangeIntegrationTest.java`:
- Around line 47-49: The test currently extracts the JWT using a fragile regex
on the response body (the line that computes token from json) then passes it to
verifier.verify(token); replace that regex extraction with proper JSON parsing:
parse the response body String (e.g., via Jackson ObjectMapper.readTree or
similar) and read the "token" field (JsonNode.get("token").asText() or Map
lookup) before calling verifier.verify(token) in
WorkerTokenExchangeIntegrationTest to make extraction robust against response
shape changes.
In
`@server/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/WorkerControlChannelIntegrationTest.java`:
- Around line 133-145: The test currently swallows any Throwable into
AtomicReference failure and only asserts non-null, which can hide unrelated
errors; change the try/catch around the
HttpClient.newBuilder().build().newWebSocketBuilder()...buildAsync(..., new
CapturingListener()).get(...) so you catch the specific
ExecutionException/CompletionException thrown by Future.get(), then inspect its
cause to assert the handshake was rejected (e.g., cause is a
WebSocketHandshakeException or the cause message/response contains "401").
Replace the broad Throwable handling on failure with a targeted assertion
against the cause of the ExecutionException coming from the buildAsync().get()
call (using jwt.token() and the same URI "/api/workers/connect" context) to
ensure the test fails only for non-handshake errors.
---
Nitpick comments:
In `@server/src/main/java/de/tum/cit/aet/hephaestus/config/KeycloakConfig.java`:
- Around line 7-8: Replace the manual SLF4J wiring in KeycloakConfig (currently
importing org.slf4j.Logger and org.slf4j.LoggerFactory and declaring a private
static final Logger) with Lombok's `@Slf4j`: remove the Logger/LoggerFactory
imports and the manual logger field, add the lombok.extern.slf4j.Slf4j
annotation to the KeycloakConfig class declaration, and use the generated log
field (log) everywhere it is referenced; ensure Lombok is available in the
project if not already.
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerTokenExchangeController.java`:
- Around line 27-55: Replace the manual Logger and explicit constructor in
WorkerTokenExchangeController with Lombok annotations: annotate the class with
`@RequiredArgsConstructor` and `@Slf4j`, remove the private static final Logger log
and the explicit public WorkerTokenExchangeController(...) constructor, keep the
final fields issuer, properties, meterRegistry (they will be injected via the
generated constructor) and preserve the failuresByIp field initializer; add the
necessary Lombok imports (lombok.RequiredArgsConstructor,
lombok.extern.slf4j.Slf4j) and remove unused LoggerFactory import.
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/session/BridgedMentorController.java`:
- Around line 44-50: Replace the manual logger and explicit constructor in
BridgedMentorController with Lombok annotations: remove the static Logger log
field and the public BridgedMentorController(MentorSessionBridge bridge)
constructor, add class-level annotations `@Slf4j` and `@RequiredArgsConstructor`,
keep the final MentorSessionBridge bridge field as-is for constructor injection,
and add the necessary Lombok imports (lombok.RequiredArgsConstructor and
lombok.extern.slf4j.Slf4j) while removing now-unused imports.
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/session/MentorSessionBridge.java`:
- Around line 39-53: Replace the manual Logger declaration and explicit
constructor in MentorSessionBridge with Lombok annotations: annotate the class
with `@Slf4j` and `@RequiredArgsConstructor`, remove the private static final Logger
log and the explicit constructor that takes WorkerSessionRegistry,
HubSessionRegistry and MeterRegistry, and make the dependency fields
(workerRegistry, sessionRegistry, sessionsOpened, noCapacity) final if not
already; keep EMITTER_TIMEOUT as-is and ensure lombok imports are added so the
class relies on constructor injection generated by `@RequiredArgsConstructor` and
logging via `@Slf4j`.
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/LoggingWorkerControlPublisher.java`:
- Around line 15-20: Replace the manual LoggerFactory logger field in
LoggingWorkerControlPublisher with Lombok's `@Slf4j`: remove the private static
final Logger log = LoggerFactory.getLogger(...) declaration and add the `@Slf4j`
annotation to the class so the existing send(WorkerControlFrame frame) method
can use the generated "log" instance; also remove unused imports for
org.slf4j.Logger and LoggerFactory if present to keep imports consistent with
the project's Lombok logging convention.
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/session/mentor/MentorSessionRunner.java`:
- Around line 25-27: The class MentorSessionRunner currently uses manual
LoggerFactory and an explicit constructor; replace these with Lombok
annotations: add `@Slf4j` to provide the logger and `@RequiredArgsConstructor` to
generate the constructor for final fields, remove the explicit Logger and
explicit constructor, and remove unused imports (org.slf4j.Logger,
org.slf4j.LoggerFactory). Also ensure other Lombok-relevant classes follow
project convention (use `@Getter/`@Setter/@Builder where required and avoid `@Data`)
and update imports to include lombok.RequiredArgsConstructor and
lombok.extern.slf4j.Slf4j as needed.
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/session/WorkerSessionDispatcher.java`:
- Around line 11-18: Replace the manual SLF4J logger and explicit constructor
wiring in WorkerSessionDispatcher with Lombok annotations: remove the private
static final Logger log = LoggerFactory... and annotate the class with `@Slf4j`
and `@RequiredArgsConstructor` (and other required Lombok annotations per
guideline if applicable); ensure any explicit constructor is removed so Lombok
generates it and update usages of "log" to use the Lombok-provided logger; apply
the same change pattern to the other classes referenced (lines 21-23) to keep
constructor and logger style consistent.
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/WorkerCapacityReporter.java`:
- Around line 22-38: The class WorkerCapacityReporter currently defines an
explicit logger field "log" and a manual constructor; replace these with Lombok
annotations by removing the private static final Logger log and the explicit
constructor, annotate the class with `@Slf4j` and `@RequiredArgsConstructor` so
Spring uses constructor injection for final fields (state, publisher, interval,
scheduler, sent, failed) and use the generated constructor and logger
throughout; ensure any use of "log" continues to work (no name change) and keep
non-final or mutable fields like "task" as-is.
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/sync/NatsProperties.java`:
- Line 61: The compact constructor in NatsProperties replaced `@NotBlank-based`
bean validation with manual isBlank checks that throw IllegalStateException;
revert to bean-validation by restoring `@NotBlank` on the affected constructor
parameters (e.g., server, username, password) and remove the manual
isBlank/IllegalStateException guards from the NatsProperties compact constructor
so Spring Boot/Validator can produce ConstraintViolationException during
property binding; ensure the class is still configured for validation (e.g.,
annotated with `@Validated/`@ConfigurationProperties as previously).
In
`@server/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerJwtTest.java`:
- Line 55: Rename test methods in WorkerJwtTest to follow the repository
convention should[ExpectedBehavior]When[Condition]: e.g. change
issuedTokenVerifiesWithExpectedClaims to
shouldVerifyIssuedTokenWhenExpectedClaimsPresent, and apply the same pattern to
the other test methods referenced in the review (rename each existing method to
start with "should", describe the expected behavior, then "When" plus the
condition). Update any usages or annotations that reference the old method names
(e.g., JUnit annotations remain the same) so the tests compile and run.
In
`@server/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerTokenExchangeIntegrationTest.java`:
- Line 30: Rename the test method validRegistrationTokenReturnsVerifiableJwt to
follow the should[ExpectedBehavior]When[Condition] pattern (for example
shouldReturnVerifiableJwtWhenRegistrationTokenIsValid) and update any references
to that method in the class; adjust the method name in
WorkerTokenExchangeIntegrationTest (the test method itself) so annotations and
imports remain intact.
In
`@server/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/hub/WorkerSessionRegistryTest.java`:
- Line 26: Rename test methods to follow the
should[ExpectedBehavior]When[Condition] convention: change
duplicateRegistrationEvictsOlderAndKeepsNewer to
shouldEvictOlderAndKeepNewerWhenDuplicateRegistration and rename the other
non-conforming test in WorkerSessionRegistryTest (the second test around the
same block) to a matching should...When... form; ensure method names start with
"should", describe the expected behavior then "When" plus the condition, and
update any references/imports accordingly.
In
`@server/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/worker/WorkerCapacityReporterTest.java`:
- Line 29: Rename the test methods to follow the
should[ExpectedBehavior]When[Condition] pattern: change
emitsCapacityReportSnapshot() to something like
shouldEmitCapacityReportSnapshotWhen[Condition] and likewise rename the other
test at the referenced location to should[ExpectedBehavior]When[Condition];
update any references/imports or annotations that rely on the method names
(e.g., the test runner), and ensure the new names are descriptive of the
expected behavior and condition for the tests in WorkerCapacityReporterTest.
In
`@server/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/worker/WorkerCapacityStateTest.java`:
- Line 13: Rename the test methods in WorkerCapacityStateTest to follow the
repository convention should[ExpectedBehavior]When[Condition]; specifically
rename reviewClaimReleaseSnapshotFlow (and the other three test methods in the
same class) to descriptive names like
shouldReviewClaimReleaseSnapshotFlowWhen[SpecificCondition] (replace
[SpecificCondition] with the actual condition each test asserts), update the
method identifiers (e.g., reviewClaimReleaseSnapshotFlow ->
shouldReviewClaimReleaseSnapshotFlowWhenX) and any usages or annotations
referencing them, and ensure method names remain valid Java identifiers and
clearly express expected behavior and trigger condition.
In
`@server/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/worker/WorkerControlChannelHealthIndicatorTest.java`:
- Line 23: Rename the test method reflectsConnectionState (and its other test
siblings in the same class) to follow the
should[ExpectedBehavior]When[Condition] pattern; for example change
reflectsConnectionState to shouldReflectConnectionStateWhenParametersProvided
(or a concise variant like shouldReportHealthWhenConnectionStateChanges) and
update any calls/annotations referencing reflectsConnectionState (e.g., display
names or parameterized test references) so the test class compiles and the
naming follows the project's should...When... convention.
In
`@server/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/worker/WorkerDrainCoordinatorTest.java`:
- Line 35: Rename the test methods to follow the repository convention
"should[ExpectedBehavior]When[Condition]": e.g. change
gracefulDrainAwaitsThenSucceeds to a name like
shouldAwaitGracefulDrainWhenConditionsMet (or similar descriptive
should...When... form), and rename the other test methods in the same class to
the same pattern; update any references/imports or annotations that refer to the
old method names so tests still run.
In
`@server/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/WorkerControlChannelIntegrationTest.java`:
- Line 73: The test method name handshakeAndCapacityRoundTrip does not follow
the repository's should[ExpectedBehavior]When[Condition] convention; rename it
to a descriptive name following that pattern (for example
shouldPerformHandshakeAndReturnCapacityWhenRoundTrip) and do the same for the
other test mentioned at line 127, update any references/usages in the class
(imports, annotations, test runner) and ensure the `@Test` methods compile and run
under the new names so consistency with the test naming guideline is preserved.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e6879c09-d874-45d5-81a5-820ad9d0ffcb
📒 Files selected for processing (84)
docker/compose.app.yamldocs/decisions/0009-worker-runtime-substrate-wss-control-channel.mdserver/pom.xmlserver/src/main/java/de/tum/cit/aet/hephaestus/SecurityConfig.javaserver/src/main/java/de/tum/cit/aet/hephaestus/agent/job/AgentJob.javaserver/src/main/java/de/tum/cit/aet/hephaestus/agent/job/AgentJobCancellationReason.javaserver/src/main/java/de/tum/cit/aet/hephaestus/agent/job/AgentJobExecutor.javaserver/src/main/java/de/tum/cit/aet/hephaestus/agent/job/AgentJobRepository.javaserver/src/main/java/de/tum/cit/aet/hephaestus/config/KeycloakConfig.javaserver/src/main/java/de/tum/cit/aet/hephaestus/config/KeycloakProperties.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/HubConfiguration.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/HubProperties.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/HubWebSocketRegistration.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/WorkerConnectedEvent.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/WorkerControlWebSocketHandler.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/WorkerDisconnectedEvent.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/WorkerSession.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/WorkerSessionRegistry.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/JavaJwtWorkerJwtVerifier.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerJwt.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerJwtHandshakeInterceptor.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerJwtInvalidException.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerJwtIssuer.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerJwtVerifier.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerKeyRing.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerSigningKey.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerTokenDenylist.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerTokenDenylistEntry.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerTokenDenylistRepository.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerTokenExchangeController.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerTokenProperties.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/package-info.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/session/BridgedMentorController.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/session/HubSessionInbox.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/session/HubSessionRegistry.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/session/MentorSessionBridge.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/LoggingWorkerControlPublisher.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/WorkerCapacityReporter.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/WorkerCapacityState.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/WorkerConfiguration.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/WorkerControlChannelHealthIndicator.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/WorkerControlClient.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/WorkerControlPublisher.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/WorkerDrainCoordinator.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/WorkerProperties.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/package-info.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/protocol/CapacityReport.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/protocol/ForceReconnect.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/protocol/FrameCodec.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/protocol/FrameEnvelope.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/protocol/Heartbeat.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/protocol/MentorSessionContext.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/protocol/SessionClose.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/protocol/SessionCloseReason.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/protocol/SessionInput.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/protocol/SessionKind.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/protocol/SessionOpen.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/protocol/SessionOutput.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/protocol/WorkerControlFrame.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/protocol/WorkerHello.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/protocol/WorkerWelcome.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/protocol/package-info.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/session/WorkerSessionDispatcher.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/session/mentor/MentorSessionRunner.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/sync/NatsProperties.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/WebhookConfiguration.javaserver/src/main/resources/application-worker.ymlserver/src/main/resources/db/changelog/1779395900727_changelog.xmlserver/src/main/resources/db/changelog/1779397290435_changelog.xmlserver/src/main/resources/db/master.xmlserver/src/test/java/de/tum/cit/aet/hephaestus/agent/job/AgentJobExecutorTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/architecture/RuntimeRoleBoundaryTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/WorkerControlChannelIntegrationTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/hub/WorkerSessionRegistryTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerJwtTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerTokenExchangeIntegrationTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/worker/WorkerCapacityReporterTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/worker/WorkerCapacityStateTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/worker/WorkerControlChannelHealthIndicatorTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/worker/WorkerDrainCoordinatorTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/worker/protocol/FrameCodecRoundTripTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/worker/session/mentor/MentorSessionRunnerTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/worker/testing/CapturingPublisher.javaserver/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/worker/testing/WorkerPropertiesFixtures.java
Comment on lines
+237
to
+249
| public void cancelInFlight(AgentJobCancellationReason reason) { | ||
| java.util.List<AgentJob> running = jobRepository.findByStatus(AgentJobStatus.RUNNING); | ||
| if (running.isEmpty()) { | ||
| return; | ||
| } | ||
| log.info("Cancelling {} in-flight job(s) with reason {}", running.size(), reason); | ||
| Instant now = Instant.now(); | ||
| String error = "worker draining"; | ||
| for (AgentJob job : running) { | ||
| try { | ||
| transactionTemplate.executeWithoutResult(status -> | ||
| jobRepository.transitionToCancelled(job.getId(), now, error, reason, Set.of(AgentJobStatus.RUNNING)) | ||
| ); |
Contributor
There was a problem hiding this comment.
Drain cancellation is not worker-scoped.
cancelInFlight loads all RUNNING jobs and can cancel work currently executing on other workers during a single-worker drain.
Proposed direction
+// Track only jobs claimed by this executor instance
+private final Set<UUID> localRunningJobIds = ConcurrentHashMap.newKeySet();
private boolean claimAndExecute(UUID jobId, Message msg) {
...
ClaimResult claim = claimed.get();
+ localRunningJobIds.add(jobId);
try {
...
} finally {
+ localRunningJobIds.remove(jobId);
heartbeat.cancel(false);
}
}
public void cancelInFlight(AgentJobCancellationReason reason) {
- java.util.List<AgentJob> running = jobRepository.findByStatus(AgentJobStatus.RUNNING);
- if (running.isEmpty()) {
+ if (localRunningJobIds.isEmpty()) {
return;
}
...
- for (AgentJob job : running) {
+ for (UUID jobId : localRunningJobIds) {
try {
transactionTemplate.executeWithoutResult(status ->
- jobRepository.transitionToCancelled(job.getId(), now, error, reason, Set.of(AgentJobStatus.RUNNING))
+ jobRepository.transitionToCancelled(jobId, now, error, reason, Set.of(AgentJobStatus.RUNNING))
);
} catch (Exception e) {
- log.warn("Failed to cancel in-flight job {}: {}", job.getId(), e.getClass().getSimpleName());
+ log.warn("Failed to cancel in-flight job {}: {}", jobId, e.getClass().getSimpleName());
}
}
}🤖 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/hephaestus/agent/job/AgentJobExecutor.java`
around lines 237 - 249, cancelInFlight currently queries all
AgentJobStatus.RUNNING jobs and cancels them globally; restrict cancellation to
this worker only by scoping the lookup or filter by the executor's worker id.
Update cancelInFlight to call a repository method that includes the worker id
(e.g., jobRepository.findByStatusAndWorkerId(AgentJobStatus.RUNNING,
thisWorkerId)) or filter the result of
jobRepository.findByStatus(AgentJobStatus.RUNNING) by job.getWorkerId(). Use the
executor's identifier (field or method on AgentJobExecutor) when selecting jobs
so transitionToCancelled is invoked only for jobs owned by this worker. Ensure
any new repository method signature matches existing repository patterns and
preserve the transactionTemplate.executeWithoutResult usage around
jobRepository.transitionToCancelled.
Comment on lines
+331
to
+346
| boolean claimed = false; | ||
| try { | ||
| claimAndExecute(jobId, msg); | ||
| claimed = claimAndExecute(jobId, msg); | ||
| } catch (SandboxCancelledException e) { | ||
| claimed = true; | ||
| handleCancellation(jobId, msg); | ||
| } catch (CannotAcquireLockException e) { | ||
| msg.nakWithDelay(Duration.ofSeconds(5)); | ||
| log.debug("Lock timeout during claim for job {}, NAK'd with 5s delay", jobId); | ||
| } catch (Exception e) { | ||
| claimed = true; | ||
| handleExecutionFailure(jobId, msg, e); | ||
| } finally { | ||
| if (claimed) { | ||
| releaseCapacity(); | ||
| } |
Contributor
There was a problem hiding this comment.
Claim-failure path releases capacity without a successful claim.
The generic catch (Exception e) sets claimed = true even for ClaimFailedException, but capacity is claimed only after a successful DB claim. This can over-release and corrupt worker capacity accounting.
Targeted fix
} catch (Exception e) {
- claimed = true;
+ claimed = !(e instanceof ClaimFailedException);
handleExecutionFailure(jobId, msg, e);
} finally {
if (claimed) {
releaseCapacity();
}📝 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.
Suggested change
| boolean claimed = false; | |
| try { | |
| claimAndExecute(jobId, msg); | |
| claimed = claimAndExecute(jobId, msg); | |
| } catch (SandboxCancelledException e) { | |
| claimed = true; | |
| handleCancellation(jobId, msg); | |
| } catch (CannotAcquireLockException e) { | |
| msg.nakWithDelay(Duration.ofSeconds(5)); | |
| log.debug("Lock timeout during claim for job {}, NAK'd with 5s delay", jobId); | |
| } catch (Exception e) { | |
| claimed = true; | |
| handleExecutionFailure(jobId, msg, e); | |
| } finally { | |
| if (claimed) { | |
| releaseCapacity(); | |
| } | |
| boolean claimed = false; | |
| try { | |
| claimed = claimAndExecute(jobId, msg); | |
| } catch (SandboxCancelledException e) { | |
| claimed = true; | |
| handleCancellation(jobId, msg); | |
| } catch (CannotAcquireLockException e) { | |
| msg.nakWithDelay(Duration.ofSeconds(5)); | |
| log.debug("Lock timeout during claim for job {}, NAK'd with 5s delay", jobId); | |
| } catch (Exception e) { | |
| claimed = !(e instanceof ClaimFailedException); | |
| handleExecutionFailure(jobId, msg, e); | |
| } finally { | |
| if (claimed) { | |
| releaseCapacity(); | |
| } |
🤖 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/hephaestus/agent/job/AgentJobExecutor.java`
around lines 331 - 346, The current catch-all sets claimed = true even when
claimAndExecute failed (e.g., ClaimFailedException), causing releaseCapacity()
to run without a prior successful claim; update the exception handling in
AgentJobExecutor so that ClaimFailedException does not set claimed to true
(either add an explicit catch (ClaimFailedException e) that sets claimed = false
and handles the failure, or modify the generic catch to check for
ClaimFailedException before setting claimed = true), keeping existing handlers
handleCancellation and handleExecutionFailure for other errors and ensuring
releaseCapacity() is only called when a real claim succeeded.
Comment on lines
+31
to
+47
| if (!keycloakProperties.isConfigured()) { | ||
| log.info("Keycloak is not configured (url/realm/clientId unset); using throwing proxy."); | ||
| return (Keycloak) java.lang.reflect.Proxy.newProxyInstance( | ||
| Keycloak.class.getClassLoader(), | ||
| new Class<?>[] { Keycloak.class }, | ||
| (proxy, method, args) -> { | ||
| // close() is benign — it's called during shutdown DisposableBeanAdapter. | ||
| if ("close".equals(method.getName()) && method.getParameterCount() == 0) { | ||
| return null; | ||
| } | ||
| throw new IllegalStateException( | ||
| "Keycloak admin client not configured (hephaestus.keycloak.{url,realm,clientId}); " + | ||
| "this role does not own user identity. Called: " + | ||
| method.getName() | ||
| ); | ||
| } | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In keycloak-admin-client 26.0.9, is org.keycloak.admin.client.Keycloakan interface or a concrete class, and can it be used withjava.lang.reflect.Proxy.newProxyInstance?
💡 Result:
In keycloak-admin-client 26.0.9, org.keycloak.admin.client.Keycloak is a concrete class, not an interface [1][2]. Regarding the use of java.lang.reflect.Proxy.newProxyInstance: 1. Mechanism Limitation: The java.lang.reflect.Proxy API is designed specifically to create dynamic proxy instances for interfaces [3][4]. It requires an array of one or more interfaces that the proxy should implement [3][5]. Because Keycloak is a concrete class, you cannot pass it to Proxy.newProxyInstance to create a proxy for that class itself [6][7]. 2. Alternative Approaches: If your goal is to intercept method calls or provide proxy-like behavior for a concrete class, you would need to use bytecode manipulation libraries such as CGLIB, ByteBuddy, or Javassist [6][7]. These libraries generate a dynamic subclass of the target concrete class at runtime, allowing for interception of non-final methods [6][7]. In summary, Proxy.newProxyInstance cannot be used directly with org.keycloak.admin.client.Keycloak because it is a concrete class [1][7].
Citations:
- 1: https://www.keycloak.org/docs-api/latest/javadocs/org/keycloak/admin/client/Keycloak.html
- 2: https://github.com/keycloak/keycloak/blob/master/integration/admin-client/src/main/java/org/keycloak/admin/client/Keycloak.java
- 3: https://docs.oracle.com/javase/7/docs/api/java/lang/reflect/Proxy.html
- 4: https://download.java.net/java/early_access/valhalla/26/docs/api/java.base/java/lang/reflect/Proxy.html
- 5: https://docs.oracle.com/javase/8/docs/api/java/lang/reflect/Proxy.html
- 6: https://stackoverflow.com/questions/7878521/dynamic-proxy-for-concrete-classes
- 7: https://medium.com/@AlexanderObregon/the-proxy-pattern-in-java-and-what-makes-it-flexible-at-runtime-ba5248a4ce01
Fix the Keycloak unconfigured fallback to not use JDK dynamic proxies for org.keycloak.admin.client.Keycloak
java.lang.reflect.Proxy.newProxyInstance can only proxy interfaces, but org.keycloak.admin.client.Keycloak is a concrete class in keycloak-admin-client 26.0.9—this fallback will fail at runtime on the “unconfigured” path.
🤖 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/hephaestus/config/KeycloakConfig.java`
around lines 31 - 47, The current fallback uses
java.lang.reflect.Proxy.newProxyInstance which only works for interfaces but
org.keycloak.admin.client.Keycloak is a concrete class, causing runtime failure;
change the fallback to return an anonymous subclass (or a small concrete stub)
of Keycloak instead of a JDK dynamic proxy: implement/override close() to be a
benign no-op and override relevant public methods to throw IllegalStateException
with the existing message. Locate the branch guarded by
keycloakProperties.isConfigured() in KeycloakConfig (the code that currently
calls Proxy.newProxyInstance) and replace that proxy creation with
constructing/returning an anonymous Keycloak subclass that preserves the same
error behavior and message.
Comment on lines
+42
to
+47
| public void revoke(String jti, Instant expiresAt) { | ||
| if (jti == null || jti.isBlank()) { | ||
| throw new IllegalArgumentException("jti must not be blank"); | ||
| } | ||
| repository.save(new WorkerTokenDenylistEntry(jti, Instant.now(), expiresAt)); | ||
| cache.put(jti, Boolean.TRUE); |
Contributor
There was a problem hiding this comment.
Validate expiresAt before saving revoke entries.
Line 46 persists expiresAt without validation; null will fail later at the DB constraint. Fail fast with an argument check.
Suggested patch
`@Transactional`
public void revoke(String jti, Instant expiresAt) {
if (jti == null || jti.isBlank()) {
throw new IllegalArgumentException("jti must not be blank");
}
+ if (expiresAt == null) {
+ throw new IllegalArgumentException("expiresAt must not be null");
+ }
repository.save(new WorkerTokenDenylistEntry(jti, Instant.now(), expiresAt));
cache.put(jti, Boolean.TRUE);
}🤖 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/hephaestus/core/runtime/hub/auth/WorkerTokenDenylist.java`
around lines 42 - 47, The revoke method in WorkerTokenDenylist saves a
WorkerTokenDenylistEntry with the provided expiresAt without validation; add an
argument check at the start of revoke (in class WorkerTokenDenylist) to ensure
expiresAt is not null (and optionally not before Instant.now() if desired),
throwing IllegalArgumentException when invalid, before calling
repository.save(new WorkerTokenDenylistEntry(...)) and cache.put(jti,
Boolean.TRUE).
Comment on lines
+58
to
+85
| public ResponseEntity<?> exchange(@RequestBody ExchangeRequest request, HttpServletRequest http) { | ||
| if (!properties.isExchangeEnabled()) { | ||
| log.warn("worker token exchange attempted but no registration token is configured"); | ||
| meterRegistry.counter(AUDIT_METRIC, "outcome", "failed", "reason", "disabled").increment(); | ||
| return ResponseEntity.status(HttpStatus.NOT_FOUND).build(); | ||
| } | ||
| String sourceIp = http.getRemoteAddr(); | ||
| AtomicInteger failures = failuresByIp.get(sourceIp, k -> new AtomicInteger(0)); | ||
| if (failures.get() >= MAX_FAILURES_PER_IP_PER_MINUTE) { | ||
| log.warn("worker token exchange throttled: too many failures from sourceIp={}", sourceIp); | ||
| meterRegistry.counter(AUDIT_METRIC, "outcome", "failed", "reason", "throttled").increment(); | ||
| return ResponseEntity.status(HttpStatus.TOO_MANY_REQUESTS).build(); | ||
| } | ||
| if (request == null || request.workerId() == null || request.workerId().isBlank()) { | ||
| failures.incrementAndGet(); | ||
| meterRegistry.counter(AUDIT_METRIC, "outcome", "failed", "reason", "bad-payload").increment(); | ||
| return ResponseEntity.status(HttpStatus.BAD_REQUEST).build(); | ||
| } | ||
| if (!constantTimeEquals(request.registrationToken(), properties.registrationToken())) { | ||
| failures.incrementAndGet(); | ||
| log.warn( | ||
| "worker token exchange rejected: bad registration token for workerId={} sourceIp={}", | ||
| request.workerId(), | ||
| sourceIp | ||
| ); | ||
| meterRegistry.counter(AUDIT_METRIC, "outcome", "failed", "reason", "bad-token").increment(); | ||
| return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build(); | ||
| } |
Contributor
There was a problem hiding this comment.
Return RFC-7807 ProblemDetail payloads for failure responses.
exchange() currently returns empty bodies for 400/401/404/429. This breaks the API’s consistent error contract and weakens client diagnostics.
Suggested direction
+import org.springframework.http.ProblemDetail;
...
- return ResponseEntity.status(HttpStatus.BAD_REQUEST).build();
+ return ResponseEntity.status(HttpStatus.BAD_REQUEST)
+ .body(ProblemDetail.forStatusAndDetail(HttpStatus.BAD_REQUEST, "workerId must not be blank"));🤖 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/hephaestus/core/runtime/hub/auth/WorkerTokenExchangeController.java`
around lines 58 - 85, The exchange method in WorkerTokenExchangeController is
returning empty responses for error cases (disabled, throttled, bad-payload,
bad-token); update each error return to include an RFC-7807 ProblemDetail
payload (use ProblemDetail.of(...) or your app's helper via
`@RestControllerAdvice`) so clients get a consistent error body; specifically
replace the ResponseEntity.status(...).build() calls after
properties.isExchangeEnabled() check, the MAX_FAILURES_PER_IP_PER_MINUTE
throttle branch, the bad-payload branch (request null/blank), and the bad-token
branch (constantTimeEquals failure) with ResponseEntitys that contain a
ProblemDetail describing the error, status, and a machine-readable "reason"
matching the existing meterRegistry tag values (disabled, throttled,
bad-payload, bad-token) and keep the existing logging, failuresByIp increments,
meterRegistry counters, and AUDIT_METRIC tags intact.
Comment on lines
+227
to
+230
| private void terminate(RunningSession session, SessionCloseReason reason) { | ||
| sessions.remove(session.sessionId); | ||
| teardown(session, reason); | ||
| } |
Contributor
There was a problem hiding this comment.
Make terminate conditional to prevent double teardown.
terminate() always calls teardown(), even if another thread already removed and closed the same session (onClose). That can double-release capacity and emit duplicate terminal frames.
Suggested fix
private void terminate(RunningSession session, SessionCloseReason reason) {
- sessions.remove(session.sessionId);
- teardown(session, reason);
+ if (sessions.remove(session.sessionId, session)) {
+ teardown(session, reason);
+ }
}📝 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.
Suggested change
| private void terminate(RunningSession session, SessionCloseReason reason) { | |
| sessions.remove(session.sessionId); | |
| teardown(session, reason); | |
| } | |
| private void terminate(RunningSession session, SessionCloseReason reason) { | |
| if (sessions.remove(session.sessionId, session)) { | |
| teardown(session, reason); | |
| } | |
| } |
🤖 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/hephaestus/core/runtime/worker/session/mentor/MentorSessionRunner.java`
around lines 227 - 230, The terminate method currently always calls teardown
which can double-tear down a session if another thread already removed/closed
it; change terminate(RunningSession session, SessionCloseReason reason) to only
call teardown when the session was actually removed from the sessions map (e.g.,
use sessions.remove(session.sessionId) and check the returned value is the same
instance or use sessions.remove(session.sessionId, session)); only invoke
teardown(session, reason) when the removal succeeded to avoid double-release and
duplicate terminal frames.
Comment on lines
+35
to
+42
| if (open.kind() == SessionKind.MENTOR_INTERACTIVE) { | ||
| mentorRunner.ifPresentOrElse( | ||
| r -> r.onOpen(open), | ||
| () -> log.warn("SessionOpen MENTOR_INTERACTIVE received but no MentorSessionRunner is wired") | ||
| ); | ||
| } else { | ||
| log.warn("Unsupported SessionKind={} on this worker; ignoring", open.kind()); | ||
| } |
Contributor
There was a problem hiding this comment.
Return an explicit close for unroutable SessionOpen frames.
When no MentorSessionRunner is wired (or kind is unsupported), this path only logs and returns. The hub never gets a terminal frame, which can leave the remote session pending indefinitely.
Suggested fix
public class WorkerSessionDispatcher {
@@
- private final Optional<MentorSessionRunner> mentorRunner;
+ private final Optional<MentorSessionRunner> mentorRunner;
+ private final WorkerControlPublisher publisher;
@@
- public WorkerSessionDispatcher(Optional<MentorSessionRunner> mentorRunner) {
+ public WorkerSessionDispatcher(Optional<MentorSessionRunner> mentorRunner, WorkerControlPublisher publisher) {
this.mentorRunner = mentorRunner;
+ this.publisher = publisher;
}
@@
r -> r.onOpen(open),
- () -> log.warn("SessionOpen MENTOR_INTERACTIVE received but no MentorSessionRunner is wired")
+ () -> {
+ log.warn("SessionOpen MENTOR_INTERACTIVE received but no MentorSessionRunner is wired");
+ publisher.send(new SessionClose(open.sessionId(), SessionCloseReason.ERROR));
+ }
);
} else {
log.warn("Unsupported SessionKind={} on this worker; ignoring", open.kind());
+ publisher.send(new SessionClose(open.sessionId(), SessionCloseReason.ERROR));
}
}🤖 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/hephaestus/core/runtime/worker/session/WorkerSessionDispatcher.java`
around lines 35 - 42, WorkerSessionDispatcher currently only logs when an
incoming SessionOpen (open) is unroutable (either open.kind() !=
SessionKind.MENTOR_INTERACTIVE or mentorRunner is empty) and never returns a
terminal frame; update the error paths to send an explicit SessionClose back to
the hub so the remote session is terminated. Concretely, in the
WorkerSessionDispatcher branch where mentorRunner.ifPresentOrElse() logs the
missing runner and in the else branch for unsupported kinds, construct and
dispatch a SessionClose (including session id from open, a clear reason string
and appropriate close code) via the same session/sender mechanism used elsewhere
in this class so the hub receives a terminal frame instead of leaving the
session pending. Ensure you use the existing MentorSessionRunner/onOpen flow
unchanged when present.
Comment on lines
+133
to
+145
| AtomicReference<Throwable> failure = new AtomicReference<>(); | ||
| try { | ||
| HttpClient.newBuilder() | ||
| .build() | ||
| .newWebSocketBuilder() | ||
| .header("Authorization", "Bearer " + jwt.token()) | ||
| .buildAsync(URI.create("ws://localhost:" + port + "/api/workers/connect"), new CapturingListener()) | ||
| .get(10, java.util.concurrent.TimeUnit.SECONDS); | ||
| } catch (Throwable t) { | ||
| failure.set(t); | ||
| } | ||
| assertThat(failure.get()).as("revoked-JWT upgrade must fail; 401 manifests as a build-async error").isNotNull(); | ||
| } |
Contributor
There was a problem hiding this comment.
Make revoked-JWT assertion specific to handshake rejection.
Catching Throwable and asserting non-null can pass on unrelated failures (e.g., networking glitches), weakening this test’s signal.
✅ Proposed tightening
- AtomicReference<Throwable> failure = new AtomicReference<>();
- try {
- HttpClient.newBuilder()
- .build()
- .newWebSocketBuilder()
- .header("Authorization", "Bearer " + jwt.token())
- .buildAsync(URI.create("ws://localhost:" + port + "/api/workers/connect"), new CapturingListener())
- .get(10, java.util.concurrent.TimeUnit.SECONDS);
- } catch (Throwable t) {
- failure.set(t);
- }
- assertThat(failure.get()).as("revoked-JWT upgrade must fail; 401 manifests as a build-async error").isNotNull();
+ Throwable thrown = org.assertj.core.api.Assertions.catchThrowable(() ->
+ HttpClient.newBuilder()
+ .build()
+ .newWebSocketBuilder()
+ .header("Authorization", "Bearer " + jwt.token())
+ .buildAsync(URI.create("ws://localhost:" + port + "/api/workers/connect"), new CapturingListener())
+ .get(10, java.util.concurrent.TimeUnit.SECONDS)
+ );
+ assertThat(thrown)
+ .as("revoked-JWT upgrade must fail during WebSocket handshake")
+ .hasRootCauseInstanceOf(java.net.http.WebSocketHandshakeException.class);🤖 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/hephaestus/core/runtime/WorkerControlChannelIntegrationTest.java`
around lines 133 - 145, The test currently swallows any Throwable into
AtomicReference failure and only asserts non-null, which can hide unrelated
errors; change the try/catch around the
HttpClient.newBuilder().build().newWebSocketBuilder()...buildAsync(..., new
CapturingListener()).get(...) so you catch the specific
ExecutionException/CompletionException thrown by Future.get(), then inspect its
cause to assert the handshake was rejected (e.g., cause is a
WebSocketHandshakeException or the cause message/response contains "401").
Replace the broad Throwable handling on failure with a targeted assertion
against the cause of the ExecutionException coming from the buildAsync().get()
call (using jwt.token() and the same URI "/api/workers/connect" context) to
ensure the test fails only for non-handshake errors.
A brutal Loop-3 audit found one real correctness bug Loop 2 had flagged but
not fixed, plus eight more bloat / over-configuration items. All addressed.
Correctness:
- WorkerControlClient connect/welcome race fixed. The FSM previously
entered the silence-watch loop checking `connected.get()`, which only
flips on inbound WorkerWelcome — if the welcome was slow to arrive, the
loop exited immediately and tore down a perfectly good socket. Added a
CountDownLatch reset on each `openWebSocket()` entry; counted down in
the WorkerWelcome handler. The FSM now waits up to `handshakeTimeout`
for the latch and reconnects only on real timeout.
Over-configuration killed:
- HubProperties shrunk from 6 fields to 3. `path`, `helloTimeout`,
`sendBufferSizeBytes` had one correct value each and nobody would tune
them; they're now `public static final` constants on the record. Saved
~20 LOC of validator + 3 lines of redundant Javadoc. Kept:
`forceReconnectThreshold` (tied to JWT TTL), `maxFrameSizeBytes`,
`sendTimeLimit`.
Overengineering killed:
- WorkerControlChannelGaugeBinder — 21-LOC static nested class whose
*only* purpose was to keep a Micrometer gauge source from GC. Deleted.
Use `Gauge.builder(...).strongReference(true)` inline in the publisher
factory methods. Same `strongReference(true)` flag applied to the six
capacity gauges; that path also went from 26 LOC of copy-paste
`Gauge.builder(...).description(...).tag(...).register(...)` blocks to
10 LOC of an iterated record + forEach.
- WorkerSessionDispatcher dropped `Optional<MentorSessionRunner>` — the
runner is unconditionally produced by WorkerConfiguration, so the
`ifPresent`/`ifPresentOrElse` chain was dead defensive code. Plain
field; the "no runner wired" warn is deleted.
Defensive theatre killed:
- WorkerControlClient inbound switch had 4 separate arms for hub-source
frames the hub never originates (Heartbeat, WorkerHello, CapacityReport,
SessionOutput) — each with its own 3-line warn block. Collapsed to a
single warnSourceMismatch helper. The Heartbeat arm's special-case
"should not occur" comment is gone too — the case is genuinely
source-mismatched, same as the others.
- `Objects.requireNonNull` on a Spring-injected param removed in Loop 2
was the last instance; no new ones added here.
Theatre / log noise:
- WorkerCapacityReporter.tick: `WARN` with full stack trace on every
failed send → `DEBUG` no-trace. A disconnected hub produces one of
these every 20 s; the `worker.heartbeats.failed` counter is the
operator surface, the stack trace is dashboard pollution. Removed.
- WorkerCapacityReporter `@Order(10)` deleted. Magic number, no other
ordered listener in the substrate to sequence against, and 10 = high
priority, not low — so the annotation didn't even do what the absence
of comment implied.
Method-level cleanup:
- MentorSessionRunner: three private synonym helpers (`failOpen` /
`terminate` / `teardown`) collapsed. `terminate` (one caller) inlined
to the call site; `failOpen` renamed `rejectOpen` since it's the
no-sandbox-yet case (no subscription/sandbox to dispose of); `teardown`
is the real worker.
Test theatre killed:
- WorkerTokenExchangeIntegrationTest no longer parses the JSON response
with a regex (`.replaceAll(".*\"token\".*", "$1")` — embarrassing in a
Java repo with Jackson autowired). Binds the response body directly to
the existing `ExchangeResponse` record; request body uses
`ExchangeRequest` record too.
Robustness:
- WorkerControlChannelHealthIndicator reports `UP` with
`configured=false` when the worker is intentionally unconfigured (no
HEPHAESTUS_HUB_URL). Previously: actuator probe went red on monolith
dev pods because the indicator hit DOWN before checking whether the
bean was a fallback.
- MentorSessionContext.Limits compact-constructor validation: rejects
non-positive memoryBytes/cpus/pidsLimit. Previously a caller could pass
`-1L` and `mergeLimits` would have happily forwarded it to the sandbox.
Tests: 47 worker-substrate unit + 4 architecture tests still green.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Breaks the Modulith cycle and removes legacy variants that survived three review loops. CI was failing on ModulithVerificationTest, four arch tests, and an outdated ERD; all green now. Why the move (`core.runtime.worker.*` → `agent.runtime.worker.*`): The substrate beans coordinate the agent's runtime — drain owns AgentJobExecutor's lifecycle, mentor runner attaches the agent's sandbox, capacity state is updated on every job claim/release. Having them in core forced core to depend on agent, which combined with agent's existing dependency on the substrate created a Slice agent → Slice core → Slice agent cycle that Spring Modulith refuses to verify. Worker runtime IS the agent layer in worker mode — the package now reflects that. What stays in `core`: - core.runtime.RuntimeRole (gating enum, foundational) - core.runtime.worker.protocol.* (wire records — shared with the hub) - core.runtime.hub.* (server-side hub; consumes only protocol records) Legacy variants killed: - LoggingWorkerControlPublisher (no-op stub fallback). Deleted along with the @ConditionalOnMissingBean wiring. WorkerControlClient is now the one and only publisher; the entire WorkerConfiguration is gated on a non-empty endpoint, so monolith mode (no endpoint) wires no substrate beans at all — agent jobs continue through NATS exactly as before. - WorkerTokenProperties.signingKey (legacy single-key shortcut) + the fallback branch in WorkerKeyRing.fromConfig. Production uses the keys[] ring; legacy callers update to a one-entry ring. Same code path for everyone. - WorkerTokenDenylistEntry vs WorkerTokenDenylist (service vs entity name collision). Renamed: entity → WorkerTokenDenylist, service → WorkerTokenDenylistService. Entity snake-cases to the table name, the Modulith Tenancy SSOT parity test passes. Boilerplate / tenancy alignment: - @WorkspaceAgnostic on WorkerTokenExchangeController + BridgedMentor- Controller + WorkerTokenDenylistService — these are infra surfaces, not workspace-scoped data endpoints. - @PreAuthorize("permitAll()") on /api/workers/exchange — auth lives in the controller body (registration-token check + per-IP throttle); the explicit declaration satisfies the arch test that every @RestController endpoint declare its security stance. - WorkerJwtInvalidException now extends RuntimeException; the verifier signature drops `throws`. (Arch rule: custom exceptions are unchecked.) - WorkspaceScopedTables.GLOBAL_TABLES adds worker_token_denylist (fleet- wide, not workspace-scoped). - DataIsolationArchitectureTest.GLOBAL_ENTITIES adds WorkerTokenDenylist. - CodeQualityTest.knownCycleBreakers adds WorkerControlClient (dispatcher → MentorSessionRunner → publisher cycle is real). ERD regenerated against the post-rename schema. Boot DX: - monolith dev (no HEPHAESTUS_HUB_URL): substrate doesn't load → zero silent stubs, zero logging publisher, capacity state absent on the optional injection sites in AgentJobExecutor → unchanged NATS path. - worker pod (HEPHAESTUS_HUB_URL set + worker profile): full substrate wires on the one primary path. Tests: 139 architecture + 46 worker-substrate unit, all green. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
|
||
| @ParameterizedTest(name = "{0}") | ||
| @MethodSource("connectionStates") | ||
| void reflectsConnectionState(String label, boolean connected, Instant lastInbound, Status expected) { |
|
|
||
| @ParameterizedTest(name = "fromConfig rejects {0}") | ||
| @MethodSource("invalidRings") | ||
| void fromConfigRejectsInvalidRings(String label, WorkerTokenProperties props, String expectedMessage) { |
| * paths fire from defensive code (e.g. {@code AgentJobExecutor.releaseCapacity}) that can't | ||
| * tolerate an exception, and the early-shutdown / double-release window is real but harmless. | ||
| */ | ||
| private static void decrementIfPositive(AtomicInteger counter, String label) { |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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/hephaestus/agent/job/AgentJobExecutor.java (1)
211-218:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
awaitInFlightis not idempotent and can crash on second invocation.Line 216 calls
arriveAndDeregister()unconditionally. IfawaitInFlight(...)is called again (e.g., drain path +@PreDestroy),Phasercan throwIllegalStateExceptionfor an unregistered party.Suggested hardening
+ private final AtomicBoolean inFlightDeregistered = new AtomicBoolean(false); + public boolean awaitInFlight(Duration timeout) { if (timeout == null || timeout.isZero() || timeout.isNegative()) { return inFlight.getUnarrivedParties() <= 1; // only the executor party left } try { - int phase = inFlight.arriveAndDeregister(); - inFlight.awaitAdvanceInterruptibly(phase, timeout.toMillis(), TimeUnit.MILLISECONDS); + if (inFlightDeregistered.compareAndSet(false, true)) { + int phase = inFlight.arriveAndDeregister(); + inFlight.awaitAdvanceInterruptibly(phase, timeout.toMillis(), TimeUnit.MILLISECONDS); + } return true;🤖 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/hephaestus/agent/job/AgentJobExecutor.java` around lines 211 - 218, awaitInFlight is not idempotent because it unconditionally calls inFlight.arriveAndDeregister(), which throws IllegalStateException if called after the party has already deregistered; make the method safe to call multiple times by first checking Phaser state and only calling arriveAndDeregister when this executor party is still registered (e.g., inspect inFlight.getRegisteredParties() or maintain an atomic "deregistered" flag), and/or wrap arriveAndDeregister in a guarded try/catch that ignores IllegalStateException; ensure you only call inFlight.awaitAdvanceInterruptibly(phase, ...) when arriveAndDeregister actually executed (or handle the alternative path) so awaitInFlight becomes idempotent—update the awaitInFlight method and related control flow around inFlight, arriveAndDeregister, and awaitAdvanceInterruptibly accordingly.server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/JavaJwtWorkerJwtVerifier.java (1)
61-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix JWT.decode exception handling in JavaJwtWorkerJwtVerifier
JWT.decode(token)can throwcom.auth0.jwt.exceptions.JWTDecodeException(not a subtype ofJWTVerificationException). With the currentcatch (JWTVerificationException e)block, malformed tokens can bypass the decode error handling and escape becauseverify()only catchesWorkerJwtInvalidException.Proposed fix
DecodedJWT decoded; try { decoded = JWT.decode(token); - } catch (JWTVerificationException e) { + } catch (com.auth0.jwt.exceptions.JWTDecodeException e) { throw new WorkerJwtInvalidException("decode failed: " + e.getClass().getSimpleName(), "decode", e); }🤖 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/hephaestus/core/runtime/hub/auth/JavaJwtWorkerJwtVerifier.java` around lines 61 - 65, The catch around JWT.decode(token) in JavaJwtWorkerJwtVerifier is catching the wrong exception type; replace or add a catch for com.auth0.jwt.exceptions.JWTDecodeException (the type thrown by JWT.decode) so malformed tokens are wrapped into WorkerJwtInvalidException the same way as other decode failures; update the catch block handling JWT.decode(token) to catch JWTDecodeException and throw new WorkerJwtInvalidException("decode failed: " + e.getClass().getSimpleName(), "decode", e) so verify() continues to only handle WorkerJwtInvalidException.
♻️ Duplicate comments (6)
server/src/main/java/de/tum/cit/aet/hephaestus/agent/job/AgentJobExecutor.java (2)
340-346:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClaim-failure path still releases capacity without a successful claim.
Line 341 sets
claimed = trueforClaimFailedException, then Line 345 releases review capacity that was never claimed.Targeted fix
} catch (Exception e) { - claimed = true; + claimed = !(e instanceof ClaimFailedException); handleExecutionFailure(jobId, msg, e); } finally { if (claimed) { releaseCapacity(); }🤖 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/hephaestus/agent/job/AgentJobExecutor.java` around lines 340 - 346, The catch/finally currently sets claimed = true for all exceptions which causes releaseCapacity() to run even when a claim failed; update AgentJobExecutor so that ClaimFailedException is handled separately (or check exception type before setting the claimed flag) and only set claimed = true when the job was actually claimed successfully; keep handleExecutionFailure(jobId, msg, e) for reporting but ensure releaseCapacity() is invoked in finally only when the claimed boolean was truly set by a successful claim (references: claimed variable, ClaimFailedException handling, handleExecutionFailure(...), releaseCapacity()).
237-249:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftDrain cancellation is still not scoped to this worker.
Line 238 fetches all
RUNNINGjobs, so draining one worker can cancel jobs executing on other workers.Suggested direction
- java.util.List<AgentJob> running = jobRepository.findByStatus(AgentJobStatus.RUNNING); + java.util.List<AgentJob> running = jobRepository.findByStatusAndWorkerId( + AgentJobStatus.RUNNING, + thisWorkerId + );If no
workerIdcolumn exists, track locally claimed job IDs in this executor and cancel only those IDs.🤖 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/hephaestus/agent/job/AgentJobExecutor.java` around lines 237 - 249, cancelInFlight currently cancels all RUNNING jobs across workers because it uses jobRepository.findByStatus(AgentJobStatus.RUNNING); update it to only cancel jobs running on this executor: either query by worker id (e.g., replace findByStatus with findByStatusAndWorkerId or filter the returned list by job.getWorkerId().equals(this.workerId)) and pass only those jobs to transactionTemplate/transitionToCancelled, or if no workerId column exists implement a local Set of claimed job IDs in this AgentJobExecutor (track IDs when claiming jobs) and iterate over that set instead of the global running list so transitionToCancelled is called only for jobs owned by this worker.server/src/main/java/de/tum/cit/aet/hephaestus/agent/runtime/worker/session/mentor/MentorSessionRunner.java (1)
155-177:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the full open pipeline with rollback to avoid leaked capacity/sandbox.
buildSpec(Line 184,UUID.fromString) andsubscribeFromNow(Line 175) can still throw after claim + map insert. Those failures currently bypassrejectOpen, which can leak session capacity and potentially a created sandbox.Proposed fix
private void attachSandbox(InteractiveSandboxService svc, RunningSession session, SessionOpen open) { - MentorSessionContext context; + MentorSessionContext context; + AttachedSandbox sandbox = null; try { context = objectMapper.treeToValue(open.context(), MentorSessionContext.class); - } catch (RuntimeException e) { - log.warn("Mentor session {} has invalid context: {}", session.sessionId, e.getMessage()); - rejectOpen(session); - return; - } - InteractiveSandboxSpec spec = buildSpec(open.sessionId(), context); - AttachedSandbox sandbox; - try { + InteractiveSandboxSpec spec = buildSpec(open.sessionId(), context); sandbox = svc.attach(spec); - } catch (InteractiveSandboxException e) { - log.warn("Mentor session {} sandbox attach failed: {}", session.sessionId, e.getMessage()); - rejectOpen(session); - return; - } - // Assign the sandbox before subscribing: a synchronous emission must see session.sandbox. - session.sandbox = sandbox; - Disposable subscription = sandbox.subscribeFromNow(frame -> publishOutput(session.sessionId, frame)); - session.subscription = subscription; + // Assign before subscribe so synchronous emissions can see sandbox. + session.sandbox = sandbox; + Disposable subscription = sandbox.subscribeFromNow(frame -> publishOutput(session.sessionId, frame)); + session.subscription = subscription; + } catch (RuntimeException | InteractiveSandboxException e) { + log.warn("Mentor session {} open failed: {}", session.sessionId, e.getMessage()); + if (sandbox != null) { + try { + sandbox.close(CLOSE_GRACE); + } catch (RuntimeException closeEx) { + log.warn("Mentor session {} sandbox rollback close failed: {}", session.sessionId, closeEx.getMessage()); + } + } + rejectOpen(session); + } }Also applies to: 184-188
🤖 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/hephaestus/agent/runtime/worker/session/mentor/MentorSessionRunner.java` around lines 155 - 177, The attachSandbox pipeline can throw after resources (session claim/map insert and maybe a created sandbox) are allocated, so wrap the full sequence from object mapping through svc.attach(...) and subscribeFromNow(...) in a single try block and perform rollback in a finally/catch: if any exception occurs after svc.attach returned, ensure you call sandbox.close()/dispose() (or the appropriate teardown) and clear session.sandbox/session.subscription and call rejectOpen(session); also handle failures from buildSpec/UUID parsing and subscribeFromNow by catching Throwable, logging, rejecting the open via rejectOpen(session), and ensuring no leaked sandbox or subscription remain; keep the assignment session.sandbox = sandbox and session.subscription = subscription only after successful subscription to avoid transient-visible state.server/src/main/java/de/tum/cit/aet/hephaestus/agent/runtime/worker/session/WorkerSessionDispatcher.java (1)
33-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSend an explicit terminal close for unsupported session kinds.
On Line 37, unsupported
SessionKindis only logged and ignored. The hub can keep the session pending indefinitely unless it receives a terminal frame.Proposed fix
+import de.tum.cit.aet.hephaestus.agent.runtime.worker.WorkerControlPublisher; @@ private final MentorSessionRunner mentorRunner; + private final WorkerControlPublisher publisher; @@ - public WorkerSessionDispatcher(MentorSessionRunner mentorRunner) { + public WorkerSessionDispatcher(MentorSessionRunner mentorRunner, WorkerControlPublisher publisher) { this.mentorRunner = mentorRunner; + this.publisher = publisher; } @@ if (open.kind() == SessionKind.MENTOR_INTERACTIVE) { mentorRunner.onOpen(open); } else { log.warn("Unsupported SessionKind={} on this worker; ignoring", open.kind()); + publisher.send(new SessionClose(open.sessionId(), SessionCloseReason.ERROR)); } }🤖 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/hephaestus/agent/runtime/worker/session/WorkerSessionDispatcher.java` around lines 33 - 38, In handleOpen(SessionOpen open) inside WorkerSessionDispatcher, when open.kind() is unsupported, emit an explicit terminal SessionClose for that open.sessionId (include a short reason like "unsupported session kind") and send it over the same outbound channel/path used for session lifecycle messages instead of only logging; this ensures the hub receives a terminal frame. Locate handleOpen and create/send a SessionClose (or equivalent terminal-close message type) for the open.sessionId before returning.server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerTokenExchangeController.java (1)
60-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn RFC-7807
ProblemDetailbodies for all error branches.Line 64, Line 71, Line 76, and Line 86 currently return empty bodies, which breaks the API’s error contract consistency.
Suggested minimal fix
+import org.springframework.http.ProblemDetail; @@ - return ResponseEntity.status(HttpStatus.NOT_FOUND).build(); + return problem(HttpStatus.NOT_FOUND, "disabled", "worker token exchange is disabled"); @@ - return ResponseEntity.status(HttpStatus.TOO_MANY_REQUESTS).build(); + return problem(HttpStatus.TOO_MANY_REQUESTS, "throttled", "too many failed attempts"); @@ - return ResponseEntity.status(HttpStatus.BAD_REQUEST).build(); + return problem(HttpStatus.BAD_REQUEST, "bad-payload", "workerId must not be blank"); @@ - return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build(); + return problem(HttpStatus.UNAUTHORIZED, "bad-token", "registration token is invalid"); @@ + private static ResponseEntity<ProblemDetail> problem(HttpStatus status, String reason, String detail) { + ProblemDetail pd = ProblemDetail.forStatusAndDetail(status, detail); + pd.setProperty("reason", reason); + return ResponseEntity.status(status).body(pd); + }As per coding guidelines
server/src/main/java/**/{*Controller,*ControllerAdvice}.java: “Return consistentProblemDetailpayloads for errors from REST endpoints following RFC-7807 via@RestControllerAdvice.”🤖 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/hephaestus/core/runtime/hub/auth/WorkerTokenExchangeController.java` around lines 60 - 87, The exchange(...) method currently returns empty error bodies on all failure branches; update each branch (when properties.isExchangeEnabled() is false, when throttled via failuresByIp, when request payload is invalid, and when registration token check via constantTimeEquals fails) to return a RFC-7807 ProblemDetail body instead of an empty ResponseEntity. For each failing branch construct a ProblemDetail (e.g., ProblemDetail.forStatus(HttpStatus.X)) with a clear title and detail describing the failure (include workerId and sourceIp where appropriate), and return ResponseEntity.status(...).body(problemDetail) while leaving the existing logging, meterRegistry.counter(...) and failure increments intact; implement these changes inside the exchange method so the controller adheres to the `@RestControllerAdvice` RFC-7807 error contract.server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/session/BridgedMentorController.java (1)
55-85:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse mentor-claim authorization on bridged mentor endpoints.
open,input, andcloseare currently guarded byisAuthenticated(), which is too broad for mentor bridge access.Suggested direction
- `@PreAuthorize`("isAuthenticated()") + `@RequireMentorAccess` public ResponseEntity<SseEmitter> open(`@RequestBody` OpenSessionRequest request) { @@ - `@PreAuthorize`("isAuthenticated()") + `@RequireMentorAccess` public ResponseEntity<Void> input(`@PathVariable` String sessionId, `@RequestBody` InputRequest request) { @@ - `@PreAuthorize`("isAuthenticated()") + `@RequireMentorAccess` public ResponseEntity<Void> close(`@PathVariable` String sessionId) {As per coding guidelines
server/**/*Controller.java: “Use@RequireMentorAccessauthorization annotation for mentor routes requiringmentor_accessJWT claim.”🤖 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/hephaestus/core/runtime/hub/session/BridgedMentorController.java` around lines 55 - 85, The three bridged-mentor endpoints open(...), input(...), and close(...) use `@PreAuthorize`("isAuthenticated()") but must require the mentor_access JWT claim; replace the broad isAuthenticated() guard with the project-specific `@RequireMentorAccess` authorization annotation on each of those methods (or at the controller class level if all endpoints should share it) and add the corresponding import for RequireMentorAccess so the methods enforce mentor-claim authorization.
🧹 Nitpick comments (1)
server/src/test/java/de/tum/cit/aet/hephaestus/agent/runtime/worker/testing/CapturingPublisher.java (1)
24-26: ⚡ Quick winConsider making
lastInboundAt()configurable for time-sensitive tests.Returning
Instant.now()on every call prevents testing scenarios where you need to simulate stale inbound data (as done inWorkerControlChannelHealthIndicatorTest.downWhenInboundStaleEvenIfConnected). That test uses a mock instead of thisCapturingPublisherprecisely because it needs to control the timestamp.♻️ Proposed fix to add configurable timestamp
public final class CapturingPublisher implements WorkerControlPublisher { public final List<WorkerControlFrame> sent = new CopyOnWriteArrayList<>(); + private Instant lastInbound = Instant.now(); `@Override` public void send(WorkerControlFrame frame) { sent.add(frame); + lastInbound = Instant.now(); } `@Override` public boolean isConnected() { return true; } `@Override` public Instant lastInboundAt() { - return Instant.now(); + return lastInbound; } }🤖 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/hephaestus/agent/runtime/worker/testing/CapturingPublisher.java` around lines 24 - 26, The CapturingPublisher.lastInboundAt() currently always returns Instant.now(), which prevents controlling time in tests; modify CapturingPublisher to hold a configurable Instant field (e.g., lastInboundAtValue) with a setter and/or constructor parameter and have lastInboundAt() return that field (defaulting to Instant.now() when the field is null) so tests can inject a stale or fixed timestamp; update any test setup (or provide a fluent withLastInboundAt(...) helper) to set the desired Instant for WorkerControlChannelHealthIndicatorTest and other time-sensitive tests.
🤖 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/hephaestus/agent/runtime/worker/session/mentor/MentorSessionRunner.java`:
- Around line 121-123: The code unconditionally calls teardown(session,
SessionCloseReason.ERROR) after sessions.remove(session.sessionId), which can
cause double-teardown if another thread already removed/closed the session
(e.g., onClose). Change the logic to only call teardown when the remove actually
succeeded, e.g. use the conditional removal variant or check the returned value:
if (sessions.remove(session.sessionId, session)) { teardown(session,
SessionCloseReason.ERROR); } so teardown is executed only when this thread
successfully removed the session.
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/agent/runtime/worker/WorkerControlClient.java`:
- Around line 261-266: The handshake latch (used to wait for WorkerWelcome in
WorkerControlClient) is not released when the socket closes or errors, causing
latch.await(...) to block until handshakeTimeout even though the attempt already
failed; update the socket close/error handlers (the paths that call
forceReconnect or handle disconnection) to count down the latch so waiting
threads wake immediately—e.g., ensure the code that currently calls
forceReconnect("welcome-timeout") or handles early close/error also invokes
latch.countDown(), and guard against double-counting by using a
boolean/AtomicBoolean flag (or similar) so the latch is only released once;
apply the same change to the other handshake wait sites referenced (the other
await blocks around the other reconnection/handshake methods).
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerTokenDenylistService.java`:
- Around line 18-19: The class WorkerTokenDenylistService is missing a Spring
stereotype so it isn't discovered or scheduled; add the `@Service` annotation to
the WorkerTokenDenylistService class declaration so Spring component scanning
registers the bean (enabling injection and making the `@Scheduled` sweepExpired()
method execute); keep the existing
`@de.tum.cit.aet.hephaestus.core.WorkspaceAgnostic` and `@Scheduled` annotations
unchanged.
---
Outside diff comments:
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/agent/job/AgentJobExecutor.java`:
- Around line 211-218: awaitInFlight is not idempotent because it
unconditionally calls inFlight.arriveAndDeregister(), which throws
IllegalStateException if called after the party has already deregistered; make
the method safe to call multiple times by first checking Phaser state and only
calling arriveAndDeregister when this executor party is still registered (e.g.,
inspect inFlight.getRegisteredParties() or maintain an atomic "deregistered"
flag), and/or wrap arriveAndDeregister in a guarded try/catch that ignores
IllegalStateException; ensure you only call
inFlight.awaitAdvanceInterruptibly(phase, ...) when arriveAndDeregister actually
executed (or handle the alternative path) so awaitInFlight becomes
idempotent—update the awaitInFlight method and related control flow around
inFlight, arriveAndDeregister, and awaitAdvanceInterruptibly accordingly.
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/JavaJwtWorkerJwtVerifier.java`:
- Around line 61-65: The catch around JWT.decode(token) in
JavaJwtWorkerJwtVerifier is catching the wrong exception type; replace or add a
catch for com.auth0.jwt.exceptions.JWTDecodeException (the type thrown by
JWT.decode) so malformed tokens are wrapped into WorkerJwtInvalidException the
same way as other decode failures; update the catch block handling
JWT.decode(token) to catch JWTDecodeException and throw new
WorkerJwtInvalidException("decode failed: " + e.getClass().getSimpleName(),
"decode", e) so verify() continues to only handle WorkerJwtInvalidException.
---
Duplicate comments:
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/agent/job/AgentJobExecutor.java`:
- Around line 340-346: The catch/finally currently sets claimed = true for all
exceptions which causes releaseCapacity() to run even when a claim failed;
update AgentJobExecutor so that ClaimFailedException is handled separately (or
check exception type before setting the claimed flag) and only set claimed =
true when the job was actually claimed successfully; keep
handleExecutionFailure(jobId, msg, e) for reporting but ensure releaseCapacity()
is invoked in finally only when the claimed boolean was truly set by a
successful claim (references: claimed variable, ClaimFailedException handling,
handleExecutionFailure(...), releaseCapacity()).
- Around line 237-249: cancelInFlight currently cancels all RUNNING jobs across
workers because it uses jobRepository.findByStatus(AgentJobStatus.RUNNING);
update it to only cancel jobs running on this executor: either query by worker
id (e.g., replace findByStatus with findByStatusAndWorkerId or filter the
returned list by job.getWorkerId().equals(this.workerId)) and pass only those
jobs to transactionTemplate/transitionToCancelled, or if no workerId column
exists implement a local Set of claimed job IDs in this AgentJobExecutor (track
IDs when claiming jobs) and iterate over that set instead of the global running
list so transitionToCancelled is called only for jobs owned by this worker.
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/agent/runtime/worker/session/mentor/MentorSessionRunner.java`:
- Around line 155-177: The attachSandbox pipeline can throw after resources
(session claim/map insert and maybe a created sandbox) are allocated, so wrap
the full sequence from object mapping through svc.attach(...) and
subscribeFromNow(...) in a single try block and perform rollback in a
finally/catch: if any exception occurs after svc.attach returned, ensure you
call sandbox.close()/dispose() (or the appropriate teardown) and clear
session.sandbox/session.subscription and call rejectOpen(session); also handle
failures from buildSpec/UUID parsing and subscribeFromNow by catching Throwable,
logging, rejecting the open via rejectOpen(session), and ensuring no leaked
sandbox or subscription remain; keep the assignment session.sandbox = sandbox
and session.subscription = subscription only after successful subscription to
avoid transient-visible state.
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/agent/runtime/worker/session/WorkerSessionDispatcher.java`:
- Around line 33-38: In handleOpen(SessionOpen open) inside
WorkerSessionDispatcher, when open.kind() is unsupported, emit an explicit
terminal SessionClose for that open.sessionId (include a short reason like
"unsupported session kind") and send it over the same outbound channel/path used
for session lifecycle messages instead of only logging; this ensures the hub
receives a terminal frame. Locate handleOpen and create/send a SessionClose (or
equivalent terminal-close message type) for the open.sessionId before returning.
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerTokenExchangeController.java`:
- Around line 60-87: The exchange(...) method currently returns empty error
bodies on all failure branches; update each branch (when
properties.isExchangeEnabled() is false, when throttled via failuresByIp, when
request payload is invalid, and when registration token check via
constantTimeEquals fails) to return a RFC-7807 ProblemDetail body instead of an
empty ResponseEntity. For each failing branch construct a ProblemDetail (e.g.,
ProblemDetail.forStatus(HttpStatus.X)) with a clear title and detail describing
the failure (include workerId and sourceIp where appropriate), and return
ResponseEntity.status(...).body(problemDetail) while leaving the existing
logging, meterRegistry.counter(...) and failure increments intact; implement
these changes inside the exchange method so the controller adheres to the
`@RestControllerAdvice` RFC-7807 error contract.
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/session/BridgedMentorController.java`:
- Around line 55-85: The three bridged-mentor endpoints open(...), input(...),
and close(...) use `@PreAuthorize`("isAuthenticated()") but must require the
mentor_access JWT claim; replace the broad isAuthenticated() guard with the
project-specific `@RequireMentorAccess` authorization annotation on each of those
methods (or at the controller class level if all endpoints should share it) and
add the corresponding import for RequireMentorAccess so the methods enforce
mentor-claim authorization.
---
Nitpick comments:
In
`@server/src/test/java/de/tum/cit/aet/hephaestus/agent/runtime/worker/testing/CapturingPublisher.java`:
- Around line 24-26: The CapturingPublisher.lastInboundAt() currently always
returns Instant.now(), which prevents controlling time in tests; modify
CapturingPublisher to hold a configurable Instant field (e.g.,
lastInboundAtValue) with a setter and/or constructor parameter and have
lastInboundAt() return that field (defaulting to Instant.now() when the field is
null) so tests can inject a stale or fixed timestamp; update any test setup (or
provide a fluent withLastInboundAt(...) helper) to set the desired Instant for
WorkerControlChannelHealthIndicatorTest and other time-sensitive tests.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cee4277f-5754-4132-8705-e87d51d9f705
📒 Files selected for processing (43)
docs/contributor/erd/schema.mmdserver/src/main/java/de/tum/cit/aet/hephaestus/agent/job/AgentJobExecutor.javaserver/src/main/java/de/tum/cit/aet/hephaestus/agent/runtime/worker/WorkerCapacityReporter.javaserver/src/main/java/de/tum/cit/aet/hephaestus/agent/runtime/worker/WorkerCapacityState.javaserver/src/main/java/de/tum/cit/aet/hephaestus/agent/runtime/worker/WorkerConfiguration.javaserver/src/main/java/de/tum/cit/aet/hephaestus/agent/runtime/worker/WorkerControlChannelHealthIndicator.javaserver/src/main/java/de/tum/cit/aet/hephaestus/agent/runtime/worker/WorkerControlClient.javaserver/src/main/java/de/tum/cit/aet/hephaestus/agent/runtime/worker/WorkerControlPublisher.javaserver/src/main/java/de/tum/cit/aet/hephaestus/agent/runtime/worker/WorkerDrainCoordinator.javaserver/src/main/java/de/tum/cit/aet/hephaestus/agent/runtime/worker/WorkerProperties.javaserver/src/main/java/de/tum/cit/aet/hephaestus/agent/runtime/worker/package-info.javaserver/src/main/java/de/tum/cit/aet/hephaestus/agent/runtime/worker/session/WorkerSessionDispatcher.javaserver/src/main/java/de/tum/cit/aet/hephaestus/agent/runtime/worker/session/mentor/MentorSessionRunner.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/HubConfiguration.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/HubProperties.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/HubWebSocketRegistration.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/WorkerControlWebSocketHandler.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/JavaJwtWorkerJwtVerifier.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerJwtInvalidException.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerJwtVerifier.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerKeyRing.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerTokenDenylist.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerTokenDenylistRepository.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerTokenDenylistService.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerTokenExchangeController.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerTokenProperties.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/session/BridgedMentorController.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/worker/protocol/MentorSessionContext.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/tenancy/WorkspaceScopedTables.javaserver/src/test/java/de/tum/cit/aet/hephaestus/agent/job/AgentJobExecutorTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/agent/runtime/worker/WorkerCapacityReporterTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/agent/runtime/worker/WorkerCapacityStateTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/agent/runtime/worker/WorkerControlChannelHealthIndicatorTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/agent/runtime/worker/WorkerDrainCoordinatorTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/agent/runtime/worker/session/mentor/MentorSessionRunnerTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/agent/runtime/worker/testing/CapturingPublisher.javaserver/src/test/java/de/tum/cit/aet/hephaestus/agent/runtime/worker/testing/WorkerPropertiesFixtures.javaserver/src/test/java/de/tum/cit/aet/hephaestus/architecture/CodeQualityTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/architecture/DataIsolationArchitectureTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/architecture/RuntimeRoleBoundaryTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/WorkerControlChannelIntegrationTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerJwtTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerTokenExchangeIntegrationTest.java
Comment on lines
+261
to
+266
| if (!latch.await(properties.control().handshakeTimeout().toMillis(), TimeUnit.MILLISECONDS)) { | ||
| forceReconnect("welcome-timeout"); | ||
| throw new IOException( | ||
| "WorkerWelcome not received within " + properties.control().handshakeTimeout() | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Handshake latch should be released on early close/error paths.
If the socket closes/errors before WorkerWelcome, latch.await(...) waits until timeout even though the attempt already failed, which adds avoidable reconnect latency.
Suggested fix
private void forceReconnect(String reason) {
WebSocket ws = webSocket.getAndSet(null);
connected.set(false);
+ welcomeLatch.get().countDown();
if (ws != null) {
try {
ws.sendClose(WebSocket.NORMAL_CLOSURE, reason);
@@
public CompletionStage<?> onClose(WebSocket webSocket, int statusCode, String reason) {
log.info("Worker control channel closed: code={}, reason={}", statusCode, reason);
connected.set(false);
+ welcomeLatch.get().countDown();
return null;
}
@@
public void onError(WebSocket webSocket, Throwable error) {
log.warn("Worker control channel error: {}", error.getClass().getSimpleName());
connected.set(false);
+ welcomeLatch.get().countDown();
}Also applies to: 365-375, 427-437
🤖 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/hephaestus/agent/runtime/worker/WorkerControlClient.java`
around lines 261 - 266, The handshake latch (used to wait for WorkerWelcome in
WorkerControlClient) is not released when the socket closes or errors, causing
latch.await(...) to block until handshakeTimeout even though the attempt already
failed; update the socket close/error handlers (the paths that call
forceReconnect or handle disconnection) to count down the latch so waiting
threads wake immediately—e.g., ensure the code that currently calls
forceReconnect("welcome-timeout") or handles early close/error also invokes
latch.countDown(), and guard against double-counting by using a
boolean/AtomicBoolean flag (or similar) so the latch is only released once;
apply the same change to the other handshake wait sites referenced (the other
await blocks around the other reconnection/handshake methods).
Comment on lines
+18
to
+19
| @de.tum.cit.aet.hephaestus.core.WorkspaceAgnostic("Worker JWTs are fleet-wide, not workspace-scoped") | ||
| public class WorkerTokenDenylistService { |
Contributor
There was a problem hiding this comment.
Missing @Service annotation prevents component scanning and scheduled execution.
Without @Service (or equivalent stereotype), Spring won't discover this class via component scanning. The @Scheduled annotation on sweepExpired() will never trigger, and the bean won't be available for injection. As per coding guidelines, use @Service annotation for business logic classes.
Proposed fix
+import org.springframework.stereotype.Service;
+
/**
* Two-tier JWT revocation check: Caffeine absorbs the read storm of WSS upgrades, Postgres is the
* durable source of truth. Revocation propagates within {`@link` `#CACHE_TTL`} (5 minutes) — accepted
* for the BYO threat model where compromise is handled by rotating registration tokens rather
* than relying on near-real-time revocation.
*/
`@de.tum.cit.aet.hephaestus.core.WorkspaceAgnostic`("Worker JWTs are fleet-wide, not workspace-scoped")
+@Service
public class WorkerTokenDenylistService {📝 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.
Suggested change
| @de.tum.cit.aet.hephaestus.core.WorkspaceAgnostic("Worker JWTs are fleet-wide, not workspace-scoped") | |
| public class WorkerTokenDenylistService { | |
| import org.springframework.stereotype.Service; | |
| /** | |
| * Two-tier JWT revocation check: Caffeine absorbs the read storm of WSS upgrades, Postgres is the | |
| * durable source of truth. Revocation propagates within {`@link` `#CACHE_TTL`} (5 minutes) — accepted | |
| * for the BYO threat model where compromise is handled by rotating registration tokens rather | |
| * than relying on near-real-time revocation. | |
| */ | |
| `@de.tum.cit.aet.hephaestus.core.WorkspaceAgnostic`("Worker JWTs are fleet-wide, not workspace-scoped") | |
| `@Service` | |
| public class WorkerTokenDenylistService { |
🤖 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/hephaestus/core/runtime/hub/auth/WorkerTokenDenylistService.java`
around lines 18 - 19, The class WorkerTokenDenylistService is missing a Spring
stereotype so it isn't discovered or scheduled; add the `@Service` annotation to
the WorkerTokenDenylistService class declaration so Spring component scanning
registers the bean (enabling injection and making the `@Scheduled` sweepExpired()
method execute); keep the existing
`@de.tum.cit.aet.hephaestus.core.WorkspaceAgnostic` and `@Scheduled` annotations
unchanged.
CI failed on a Spring Security filter-chain collision and CodeRabbit / principal-engineer audit flagged HubSessionInbox as a one-impl SPI we have no excuse for shipping. 1. Lockdown chain vs resource-server chain conflict (CI blocker). `lockdownSecurityFilterChain` was gated on `@ConditionalOnMissingBean(name="resourceServerSecurityFilterChain")`. Spring evaluates that during bean-method registration, before the other chain's own `@ConditionalOnBean(JwtDecoder.class)` has resolved — so under integration profiles (Keycloak configured, JwtDecoder present) BOTH chains loaded and Spring Security refused to start with "filter chain that matches any request has already been configured". Switched the lockdown gate to the same condition the resource-server uses for absence: `@ConditionalOnMissingBean( JwtDecoder.class)`. Mutually exclusive by construction — only one any-request chain ever loads. 2. HubSessionInbox one-impl SPI deleted. The interface had one production impl (MentorSessionBridge) and one consumer (WorkerControlWebSocketHandler). The Javadoc justified it with "WSS handler compiles without a bridge (cold-start smoke tests, boot-without-mentor monolith)" — but the bridge is *already* `@ConditionalOnProperty(hub.bridge.enabled=true)`, so the handler already injects `Optional<MentorSessionBridge>`. The interface just added type-laundering between two beans that always travel together. Inlined: handler now takes `Optional<MentorSessionBridge>` directly; the bridge drops `implements HubSessionInbox`, the `@Override` annotations on `onSessionOutput` / `onSessionClose` go away, the interface file is deleted. -15 LOC, same behavior, one less type to maintain. Tests: 46 worker-substrate unit + 139 architecture + the SecurityFilterChainRuntimeTest integration suite all green. (Out of scope, flagged for follow-up: MentorAgentProperties has a parallel LLM-resolution path against AgentConfig; Practice.description shadows Practice.criteria; PracticeDetectionResultParser supports agent-supplied delivery alongside DeliveryComposer. These are real two-variant patterns but their blast radius exceeds this PR's scope — each warrants its own focused PR.) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…+ practice detection
Brutal scope expansion on top of the worker-substrate PR — the user asked to
ignore review-burden boundaries and ruthlessly eliminate every two-variant
pattern the audit found. One clean primary path for every contract.
Mentor:
- MentorRunnerClient drops the 5-arg legacy constructor; boundThreadId is
required and non-null (production already always passed it).
- MentorTurnRequest drops the 3-arg legacy constructor; tests pass null for
clientUserMessageId explicitly.
- LlmProxyAuthShell drops the 3-arg + 5-arg overloads; one 6-arg primary path
with explicit nulls at the call sites.
- MentorTurnPersistence drops the legacy ux_chat_message_in_flight
constraint-name match; only the current v2 partial-unique index name fires.
- Mentor LLM-config dual-path killed: MentorAgentProperties.{llmProvider,
credentialMode, llmApiKey, modelName, timeoutSeconds} deleted, factory
fromProperties removed. AgentConfig is the single source of truth; missing
config throws IllegalStateException with a clear message.
Practice:
- Practice.description column dropped (Liquibase migration backfills criteria
from description, then drops the column and makes criteria NOT NULL).
- PracticeDTO, CreatePracticeRequestDTO, UpdatePracticeRequestDTO no longer
carry description; criteria is @notblank.
- PracticeCatalogAspectProvider writes criteria only — the agent context no
longer ships a synthetic description field that diverged from criteria.
- PullRequestReviewHandler drops the criteria-or-description fallback;
criteria is guaranteed non-null by the schema.
Agent delivery contract:
- Agent no longer emits a top-level delivery.{mrNote,diffNotes} block.
DeliveryComposer is the single source of truth for the MR summary; agents
only supply findings and per-finding suggestedDiffNotes.
- ValidatedFinding carries suggestedDiffNotes; DeliveryComposer prefers them
over synthesized notes from evidence.locations.
- pi-runner.mjs deletes the set_review_summary tool and its prompt scaffolding;
result.json shape is { findings: [...] }.
- pi-orchestrator.md updated to describe the new single-output contract.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…tion Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…m 7-agent review Ran seven principal-engineer audits with web-search-grounded rubrics. Findings ranged from F-grade defects (prompts still telling the agent to call a tool that no longer exists) to nitpicks. This commit lands every fix needed to hit A across mentor, worker, practice, bloat, DX, migrations, and security. CRITICAL FIXES - Prompt drift: PullRequestReviewHandler, TaskEnvelopeFixtureTest fixture, task-fixtures/v1/practice-review.json, and PracticeRunnerLiveLlmTest all still told the agent to call `set_review_summary`, which was deleted from pi-runner.mjs in the prior commit. Every job would have wasted a tool turn. - Silence-deadline reconnect storm: WorkerControlClient closes the channel when the hub doesn't speak for 3× heartbeat interval, but the hub never speaks first in steady state. Hub now echoes Heartbeat(false) on every CapacityReport, anchoring lastInboundAt and stopping the per-minute reconnect churn. - Double @ConditionalOnProperty on BridgedMentorController: Spring only honors the first annotation, so the bridge gate was silently bypassed. Replaced with @ConditionalOnBean(MentorSessionBridge.class) — the bean itself is bridge-gated, so the controller can never wire when the bridge is off. - DiffHunkValidator snap-everywhere: a note at L42 was being moved to L500 if L42 wasn't in the diff. Now bounded by MAX_SNAP_DELTA=10; beyond that, dropped (with a count log). - Composer classification bug exposed by the new contract: NEGATIVE findings with a `suggestedDiffNote` but no `evidence.locations` were being marked non-inlinable and never reaching collectDiffNotes. Now the per-finding suggested note is the stronger signal. MIGRATION SAFETY (Liquibase 1779500000000) - Split into three changesets so backfill / drop / NOT-NULL can each fail or re-run independently with their own preconditions and rollback blocks. - Each step is preconditioned with onFail="MARK_RAN", making re-runs against manually patched schemas safe. - Documented forward-only rollback: data is destroyed by the drop and cannot be recreated; operators must restore from backup. - Regenerated docs/contributor/erd/schema.mmd to reflect criteria NOT NULL and description gone. DX - Replaced docs prerequisite "npm (>= 10.8)" with "pnpm (>= 11) via corepack". Was sending new contributors down a wrong-tooling rabbit hole. - Added 7 worker env vars to docker/.env.example so split-pod deployments don't require reading the source. - New RuntimeRoleStartupLogger logs which roles wired at boot, WARNs if all three are off (current behavior: silent do-nothing JVM). SECURITY - WorkerTokenExchangeController now honors X-Forwarded-For for per-IP rate limiting (compose-deployed proxies were collapsing all clients to one IP), emits Cache-Control: no-store on JWT responses, and logs successful exchanges with workerId+jti+sourceIp+exp for audit symmetry. - EncryptedStringConverter hoists SecureRandom to a static-final to avoid per-encrypt seeding stalls. TEST + COMMENT BLOAT - Deleted `noDedupWhenAllUnique` (subsumed by sibling), `preservesValidEscapes` + `noOpWithoutBackslashes` (testing implementation identity, not behavior). - Removed dead `// guidanceMethod removed` and `(#1071)` issue-ref comments. LIVE VERIFICATION - 32 integration tests passing (worker WSS handshake, full practice pipeline, mentor persistence under real Postgres, JWT exchange). - 139 architecture tests passing. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/main/java/de/tum/cit/aet/hephaestus/agent/handler/PullRequestReviewHandler.java (1)
405-413:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce diff-scope after note correction to avoid invalid inline posts.
On Line 407, corrected notes are accepted as-is. Since unknown-file notes are not dropped by the validator, out-of-diff notes can still be sent to the provider API and fail delivery.
Suggested patch
if (!validLines.isEmpty()) { var correctedNotes = DiffHunkValidator.validateAndCorrect( delivery.diffNotes(), validLines, job.getId().toString() ); + correctedNotes = correctedNotes + .stream() + .filter(note -> validLines.containsKey(note.filePath())) + .toList(); delivery = new PracticeDetectionResultParser.DeliveryContent(delivery.mrNote(), correctedNotes); }🤖 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/hephaestus/agent/handler/PullRequestReviewHandler.java` around lines 405 - 413, The corrected notes returned by DiffHunkValidator.validateAndCorrect can still contain out-of-diff or unknown-file entries, so after computing validLines (via computeDiffValidLines(job)) and after getting correctedNotes from DiffHunkValidator.validateAndCorrect(delivery.diffNotes(), validLines, job.getId().toString()), filter correctedNotes against validLines and drop any note whose file/line is not present in validLines (and drop unknown-file notes) before constructing the new PracticeDetectionResultParser.DeliveryContent(delivery.mrNote(), ...); update the variable delivery to use this filtered list so only in-diff notes are kept for provider delivery.
🤖 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/hephaestus/core/runtime/hub/auth/WorkerTokenExchangeController.java`:
- Around line 103-118: resolveSourceIp currently accepts any left-most
X-Forwarded-For and is used by exchange to key failure counters
(MAX_FAILURES_PER_IP_PER_MINUTE), allowing attackers to rotate XFF to bypass
throttling; change resolveSourceIp to only trust X-Forwarded-For when the
immediate peer (http.getRemoteAddr()) is a known/trusted proxy (e.g., loopback,
configured proxy CIDRs or a trustedProxies set), otherwise ignore XFF and return
getRemoteAddr(); update any config or helper used by exchange to consult that
trustedProxies list so untrusted peers cannot inject XFF values.
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/RuntimeRoleStartupLogger.java`:
- Around line 20-26: The class RuntimeRoleStartupLogger currently declares a
manual Logger field (log) and an explicit constructor that assigns the
Environment field; replace these with Lombok annotations by removing the
LoggerFactory.getLogger(...) declaration and the explicit constructor and
annotating the class with `@Slf4j` and `@RequiredArgsConstructor` so Lombok provides
the logger and the constructor that injects the final Environment field
(environment).
---
Outside diff comments:
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/agent/handler/PullRequestReviewHandler.java`:
- Around line 405-413: The corrected notes returned by
DiffHunkValidator.validateAndCorrect can still contain out-of-diff or
unknown-file entries, so after computing validLines (via
computeDiffValidLines(job)) and after getting correctedNotes from
DiffHunkValidator.validateAndCorrect(delivery.diffNotes(), validLines,
job.getId().toString()), filter correctedNotes against validLines and drop any
note whose file/line is not present in validLines (and drop unknown-file notes)
before constructing the new
PracticeDetectionResultParser.DeliveryContent(delivery.mrNote(), ...); update
the variable delivery to use this filtered list so only in-diff notes are kept
for provider delivery.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc1e35bb-e884-48ce-b9ec-e59078d268d3
📒 Files selected for processing (22)
docker/.env.exampledocs/contributor/erd/schema.mmddocs/contributor/local-development.mdxserver/src/main/java/de/tum/cit/aet/hephaestus/agent/handler/DeliveryComposer.javaserver/src/main/java/de/tum/cit/aet/hephaestus/agent/handler/DiffHunkValidator.javaserver/src/main/java/de/tum/cit/aet/hephaestus/agent/handler/PullRequestReviewHandler.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/RuntimeRoleStartupLogger.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/HubConfiguration.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/WorkerControlWebSocketHandler.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/auth/WorkerTokenExchangeController.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/hub/session/BridgedMentorController.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/security/EncryptedStringConverter.javaserver/src/main/resources/agent/pi-runner.mjsserver/src/main/resources/db/changelog/1779500000000_changelog.xmlserver/src/test/java/de/tum/cit/aet/hephaestus/agent/handler/DiffHunkValidatorTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/agent/handler/PracticeDetectionPipelineIntegrationTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/agent/handler/PracticeDetectionResultParserTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/agent/practice/live/PracticeRunnerLiveLlmTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/agent/task/TaskEnvelopeFixtureTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/architecture/DataIsolationArchitectureTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/architecture/RuntimeRoleBoundaryTest.javaserver/src/test/resources/task-fixtures/v1/practice-review.json
💤 Files with no reviewable changes (1)
- server/src/test/java/de/tum/cit/aet/hephaestus/agent/handler/PracticeDetectionResultParserTest.java
✅ Files skipped from review due to trivial changes (2)
- server/src/test/resources/task-fixtures/v1/practice-review.json
- docs/contributor/local-development.mdx
Comment on lines
+103
to
+118
| /** | ||
| * Prefer the left-most {@code X-Forwarded-For} entry when the request came through a | ||
| * reverse proxy (Coolify / Traefik / Nginx). Falls back to the raw socket peer. Without | ||
| * this, the per-IP rate limit is a per-proxy rate limit and one bad actor exhausts the | ||
| * counter for the entire fleet. | ||
| */ | ||
| static String resolveSourceIp(HttpServletRequest http) { | ||
| String xff = http.getHeader("X-Forwarded-For"); | ||
| if (xff != null && !xff.isBlank()) { | ||
| int comma = xff.indexOf(','); | ||
| String first = (comma >= 0 ? xff.substring(0, comma) : xff).trim(); | ||
| if (!first.isEmpty()) { | ||
| return first; | ||
| } | ||
| } | ||
| return http.getRemoteAddr(); |
Contributor
There was a problem hiding this comment.
Don't trust raw X-Forwarded-For for throttling.
resolveSourceIp() accepts any left-most X-Forwarded-For value, and exchange() keys the failure counter on that string. A caller can rotate this header to bypass MAX_FAILURES_PER_IP_PER_MINUTE entirely. Only use forwarded headers after a trusted proxy has canonicalized them; otherwise fall back to the socket peer address here.
🤖 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/hephaestus/core/runtime/hub/auth/WorkerTokenExchangeController.java`
around lines 103 - 118, resolveSourceIp currently accepts any left-most
X-Forwarded-For and is used by exchange to key failure counters
(MAX_FAILURES_PER_IP_PER_MINUTE), allowing attackers to rotate XFF to bypass
throttling; change resolveSourceIp to only trust X-Forwarded-For when the
immediate peer (http.getRemoteAddr()) is a known/trusted proxy (e.g., loopback,
configured proxy CIDRs or a trustedProxies set), otherwise ignore XFF and return
getRemoteAddr(); update any config or helper used by exchange to consult that
trustedProxies list so untrusted peers cannot inject XFF values.
Comment on lines
+20
to
+26
| private static final Logger log = LoggerFactory.getLogger(RuntimeRoleStartupLogger.class); | ||
|
|
||
| private final Environment environment; | ||
|
|
||
| public RuntimeRoleStartupLogger(Environment environment) { | ||
| this.environment = environment; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/RuntimeRoleStartupLogger.java"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE"
exit 1
fi
echo "=== File head ==="
wc -l "$FILE"
sed -n '1,120p' "$FILE" | nl -ba
echo "=== Imports ==="
rg -n "^(import|package|public class|@|private static final|LoggerFactory|Logger|RequiredArgsConstructor|Slf4j)" "$FILE" || true
echo "=== Logger usage ==="
rg -n "log\\.|LoggerFactory|getLogger\\(" "$FILE" || trueRepository: ls1intum/Hephaestus
Length of output: 236
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="server/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/RuntimeRoleStartupLogger.java"
echo "=== File: $FILE ==="
wc -l "$FILE"
echo "=== First 120 lines (with line numbers via cat -n) ==="
cat -n "$FILE" | sed -n '1,120p'
echo "=== Imports ==="
rg -n "^(import|package|public class|@|private static final|LoggerFactory|Logger|RequiredArgsConstructor|Slf4j)" "$FILE" || true
echo "=== LoggerFactory/getLogger/log usage ==="
rg -n "LoggerFactory|getLogger\\(|\\blog\\." "$FILE" || true
echo "=== Constructor / injection ==="
rg -n "public RuntimeRoleStartupLogger\\(|RuntimeRoleStartupLogger\\(" "$FILE" || true
rg -n "Environment environment|private final Environment" "$FILE" || trueRepository: ls1intum/Hephaestus
Length of output: 3594
Use Lombok @Slf4j + @RequiredArgsConstructor in RuntimeRoleStartupLogger
Replace the manual LoggerFactory logger and explicit constructor injection with project-standard Lombok annotations.
♻️ Proposed refactor
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
@@
`@Component`
+@Slf4j
+@RequiredArgsConstructor
public class RuntimeRoleStartupLogger {
-
- private static final Logger log = LoggerFactory.getLogger(RuntimeRoleStartupLogger.class);
private final Environment environment;
-
- public RuntimeRoleStartupLogger(Environment environment) {
- this.environment = environment;
- }📝 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.
Suggested change
| private static final Logger log = LoggerFactory.getLogger(RuntimeRoleStartupLogger.class); | |
| private final Environment environment; | |
| public RuntimeRoleStartupLogger(Environment environment) { | |
| this.environment = environment; | |
| } | |
| `@Component` | |
| `@Slf4j` | |
| `@RequiredArgsConstructor` | |
| public class RuntimeRoleStartupLogger { | |
| private final Environment environment; |
🤖 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/hephaestus/core/runtime/RuntimeRoleStartupLogger.java`
around lines 20 - 26, The class RuntimeRoleStartupLogger currently declares a
manual Logger field (log) and an explicit constructor that assigns the
Environment field; replace these with Lombok annotations by removing the
LoggerFactory.getLogger(...) declaration and the explicit constructor and
annotating the class with `@Slf4j` and `@RequiredArgsConstructor` so Lombok provides
the logger and the constructor that injects the final Environment field
(environment).
Two related fixes from a live-test attempt against the branch: CI (was failing on main): KeycloakPropertiesTest + NatsPropertiesTest were stale. The bloat-surgery commit (8ad5055) deliberately dropped @notblank from KeycloakProperties so worker/webhook pods can boot without Keycloak, and gated NatsProperties validation on `enabled=true` so disabled-NATS overlays don't need a sentinel `nats://disabled` value. The tests still expected the old "validation always fires" contract. Updated them to assert the new behaviour: KeycloakProperties.isConfigured() returns false for blank URL/realm/clientId, and disabled NATS tolerates a blank server. DX (caught by the live-test agent): a fresh dev box with no Keycloak running falls back to lockdownSecurityFilterChain — which had anyRequest().denyAll(). Setting hephaestus.dev.trigger-enabled=true had no effect because the permit lived on the resource-server chain only. The lockdown chain now also honors the flag (and CORS preflight), so single-pod smoke tests can trigger reviews without spinning up Keycloak. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
DX audit (B-→B) found HubProperties had three keys nobody will ever tune: `maxFrameSizeBytes`, `sendTimeLimit`, `forceReconnectThreshold`. They are protocol-internal constants — tied to the WSS frame size, the JWT lifetime, and the worker's silence-deadline reconnect logic. Operator config surface that can't be tuned sanely is worse than constants. Collapsed `HubProperties` from a `@ConfigurationProperties` record to a package-private constants holder. Removed the `@EnableConfigurationProperties` registration and the constructor injection on `WorkerControlWebSocketHandler`. Filled the runtime-roles.mdx gap the previous audit flagged. Documents the three-role model, monolith vs split-pod tradeoffs, per-overlay matrix, worker-pod env-var minimum set, and drain semantics. Links to the relevant ADRs (0005, 0008, 0009). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Docs CI failed on the new admin/runtime-roles.mdx: relative links into ../decisions/ are GitHub-only paths that docusaurus can't resolve. Reference the ADRs by filename instead and add the page to sidebars.admin.ts. Verified locally: `pnpm --filter docs run build` succeeds. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…erty bypasses These were the pre-existing blockers preventing end-to-end practice review in the live-test path (caught by the prior live-test agent). NODE_PATH for the agent-pi runner: - pnpm 11 installs globals into a content-hashed path `/usr/local/pnpm/global/v11/<hash>/node_modules`. The runner at `/workspace/.run-pi.mjs` does `import "@earendil-works/pi-coding-agent"` — Node walks /workspace/node_modules then /node_modules and finds nothing, dying with ERR_MODULE_NOT_FOUND before reaching the LLM. - Dockerfile now symlinks the resolved install path to a stable location `/opt/pi-sdk/node_modules` (the hash changes whenever the lockfile changes, so the symlink is computed at build-time from the actual install). - PiRuntimeFactory.nodeEnvFor() sets NODE_PATH to that stable path so the runner resolves the SDK from anywhere it's executed. Three silent gate bypasses — same antipattern as the BridgedMentorController double-annotation bug fixed earlier. Spring honors only the FIRST @ConditionalOnProperty on an element; the second is silently ignored: - AgentJobExecutor: had agent.nats.enabled=true plus runtime.worker.enabled stacked — the worker-role check never fired. - AgentNatsConsumerConfig: same two-annotation stack, same bypass. - DockerSandboxConfiguration: sandbox.enabled=true plus worker.enabled stacked — sandbox would wire even when explicitly off on a server-only pod if worker.enabled was the dominant intended check. All three now use a single @ConditionalOnExpression combining both predicates. Added a RuntimeRoleBoundaryTest assertion that fails any future class with multiple stacked @ConditionalOnProperty annotations, so this antipattern cannot land again without a CI failure. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…es NODE_PATH) Live-test agent caught my mistake: NODE_PATH only governs the legacy CommonJS require() path. Node's ESM import resolver ignores it entirely. The pi-runner.mjs uses ESM imports, so setting NODE_PATH was a no-op and the runner still died with ERR_MODULE_NOT_FOUND. Real fix: the in-container startup command already symlinks /workspace/node_modules → somewhere. Point that symlink at the stable /opt/pi-sdk/node_modules (Dockerfile-built symlink to pnpm's content- addressed install) instead of /usr/local/lib/node_modules (which only holds npm + corepack). ESM's upward-walk from /workspace/.run-pi.mjs then finds @earendil-works/pi-coding-agent at /workspace/node_modules/ @earendil-works/pi-coding-agent. Dropped the NODE_PATH env var — pointless for ESM, just confusing. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The Pi SDK's default openai provider posts to /v1/responses (Responses API). Some endpoints (TUM GPU gateway, on-prem OpenAI-compatible deployments) only speak /v1/chat/completions. The codebase already had a hephaestus custom provider extension that routes via chat/completions when a baseUrl is set on PiPlanSpec — but the only way to set that baseUrl was the worker-pod-level hephaestus.worker.llm.base-url override. In a monolith / single-pod boot, there was no surface to express "this workspace's LLM endpoint needs the chat/completions provider," so live practice reviews against TUM GPU hit /v1/responses, got 405 Method Not Allowed, and died with zero findings. Caught by the live-test agent. This adds llm_base_url to AgentConfig: - Liquibase migration 1779600000000 (idempotent, with rollback) - Entity field + getter/setter (length 512, nullable) - ConfigSnapshot schema v4 — bumped, threaded into PracticeAgentRequest via AgentJobExecutor (worker-level override still wins) - CreateAgentConfigRequestDTO + UpdateAgentConfigRequestDTO surface the field; empty string clears it on update - AgentConfigDTO exposes it back to admin UI - ERD + openapi.yaml + webapp api client regenerated Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… ids Pi's model resolver parses the first slash in a model id as a `provider/model` reference. For TUM-style ids like `openai/gpt-oss-120b` that means Pi looks up the built-in `openai` provider, fails to find the model, and SILENTLY falls back to the OpenAI default (`gpt-5.4` against api.openai.com) — bypassing the hephaestus extension entirely and authenticating with the wrong endpoint's key. Fix: when registering the hephaestus custom provider, strip the leading `<provider>/` segment from the model id. PiRuntimeFactory now writes `defaultModel = "hephaestus/<bare-id>"` to settings.json, and LlmProxyAuthShell exports `PI_HEPHAESTUS_MODEL=<bare-id>` to the extension. Pi parses `hephaestus/gpt-oss-120b` unambiguously and routes to the right gateway. Also fixed pi-runner.mjs to log `event.message.errorMessage` alongside `stopReason` — without it the live-test agent burned an hour chasing the fallback before discovering it was a 401 from api.openai.com. New PiRuntimeFactoryTest case `slashPrefixedModelIdCollapses` locks in the strip semantics so this can't silently regress. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…s/ prefix Two corrections on top of acc060a after deeper analysis of the Pi SDK: 1. createAgentSession does auto-load extensions via resourceLoader.reload() (verified in dist/core/sdk.js:96). The previous theory that extensions weren't being loaded was wrong. Surface the extensionsResult diagnostics on the runner so silent load failures (the actual class of bug here) are visible in container logs going forward. 2. defaultModel must be the BARE model id, not hephaestus/<id>. Pi's resolver calls modelRegistry.find(defaultProvider, defaultModel) with both args separately — defaultProvider already supplies the "which provider" disambiguation. Adding the hephaestus/ prefix made find() look up a model whose id literally contained the slash, which never matches anything in the extension's registration. Test updated to lock in the bare-id semantics. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ension Pi's built-in OpenAI provider auto-activates on OPENAI_API_KEY presence and wins resolution over our custom hephaestus extension. Result: the gateway-issued key (e.g. TUM GPU's sk-caf5d...) gets sent to api.openai.com, which returns 401 with no useful diagnostic. Confirmed by the live-test agent: extension loaded successfully, env var was set per "backwards compatibility", and every request went to OpenAI's production endpoint. Fix: in API_KEY+baseUrl mode, ONLY export PI_HEPHAESTUS_* env. The hephaestus extension reads its own credentials via PI_HEPHAESTUS_API_KEY; the built-in OpenAI provider has no auth and falls out of consideration. Without-baseUrl mode is unchanged (single-provider path). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…into pi-runner Two coupled fixes after the previous live-test surfaced "No API key found for the selected model": 1. AuthStorage + ModelRegistry must be constructed and passed to createAgentSession. The hephaestus extension registers its provider with apiKey: "PI_HEPHAESTUS_API_KEY" (an env-var name, indirected); without an AuthStorage backing the ModelRegistry the lookup at session-start can't resolve that name to the actual key, throws, and the agent dies before any HTTP call. 2. Reverted stripProviderPrefix. The previous round mistakenly stripped the leading provider segment from the model id, which broke gateways like TUM GPU that require the full openai/gpt-oss-120b on the wire. With defaultProvider="hephaestus" pinned explicitly in settings.json, Pi doesn't reinterpret slashes in defaultModel — pass it verbatim. Tests updated to reflect both fixes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…SDK race Pi 0.74.1 has a race in createAgentSession: findInitialModel runs BEFORE ExtensionRunner.bindRuntimeContext drains the extension's pending provider registrations into the ModelRegistry. With the hephaestus provider registered only via the extension factory, the session is created with model=undefined and the agent crashes at first stream call with "No API key found for the selected model" (because _getRequiredRequestAuth(undefined).provider === "unknown"). Register the same provider definition directly on the ModelRegistry inside pi-runner.mjs before createAgentSession is called. The extension file remains as the future-proof path for any CLI/TUI invocation, but the embedded SDK runner no longer depends on the race resolving in time. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Three principal-engineer-driven refactors to remove band-aids that accumulated across the worker runtime substrate epic (#1099) + WSS control channel (#1098): 1. Single source of truth for the hephaestus Pi provider. Previously, `pi-runner.mjs` registered the provider on the ModelRegistry directly (to dodge the Pi 0.74.x findInitialModel race) AND `PiRuntimeFactory` emitted parallel `provider-openai.ts` / `provider-anthropic.ts` extension files. Two definitions of contextWindow, maxTokens, cost, and api — guaranteed to drift. Now the runner-script registration is the only definition; the mentor runner gets the same direct- registration pattern; the TS files are deleted; `buildExtensionFile()` is removed. Settings.json still pins `defaultProvider="hephaestus"` so Pi's resolver sees the runtime registration. 2. ConfigSnapshot.SCHEMA_VERSION reverted 4 → 3. The bump was added when `llmBaseUrl` was introduced, but additive nullable fields are forward- and backward-compatible thanks to `@JsonIgnoreProperties(ignoreUnknown = true)` and Jackson's default null-fill. Documented the discipline: bump only on breaking changes. 3. Deterministic Pi SDK install path. The Dockerfile previously did `pnpm add -g` then fished out the install via `ls -1d /usr/local/pnpm/global/v11/*/node_modules | head -n 1` — fragile because pnpm 11 stores globals under a content-hashed path. Replaced with `npm install --prefix /opt/pi-sdk` which writes a stable, hash-free `/opt/pi-sdk/node_modules/@earendil-works/ pi-coding-agent` directory. PiRuntimeFactory's symlink chain (workspace/node_modules → /opt/pi-sdk) is unchanged. Plus: consolidated four branch-added Liquibase changelogs (1779395900727, 1779397290435, 1779500000000, 1779600000000) into one `1779520390544_worker_runtime_substrate.xml` with precise millisecond timestamp and `onFail=MARK_RAN` preconditions on every changeset so partial re-applies across sibling worktrees stay idempotent. Bumped `agent_config.llm_base_url` column width 512 → 2048. Live e2e verified after all four changes: job COMPLETED, exit_code=0, 8 LLM calls to openai/gpt-oss-120b at TUM GPU through the runner-script-registered hephaestus provider, snapshot stored at schemaVersion=3. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Three principal-engineer subagents audited every speculative surface against open issues #1100, #1106, #1133, #1138, #1159, #1161 and the local mentor runtime trajectory. Cuts below are evidence-based, not opinion-based. Deletes (~1150 LoC): 1. hub/session/ bridge package + worker-side mentor session protocol - MentorSessionBridge, HubSessionRegistry, BridgedMentorController - WorkerSessionDispatcher, MentorSessionRunner - SessionOpen, SessionInput, SessionOutput, SessionClose, SessionKind, SessionCloseReason, MentorSessionContext - MentorSessionRunnerTest + the bridge.enabled property + /api/mentor/bridge security path Reason: Epic #1099 explicitly carved this out ("Mentor session frames: defer"). #1106 ("move mentor sessions onto workers") is unassigned, no milestone, and its sub-issues (#1157 MentorSessionRouter SPI, #1159 MentorSessionManager + FrameRingBuffer, #1161 MentorBridge) refactor the SPI on MentorChatService, not on top of this bridge surface. The branch's bridge is gated default-off via a never-set flag and would be rewritten when #1106 lands. 2. WorkerControlPublisher interface (single impl: WorkerControlClient). #1106 explicitly defers NATS-based cross-pod fanout, so the iface has no second consumer in the roadmap. Tests refactored to mock the concrete class. 3. WorkerConnectedEvent (zero listeners across all open issues; only WorkerDisconnectedEvent is consumed by #1138 lease-release, kept). 4. AgentJobCancellationReason.USER, AgentJobCancellationReason.TIMEOUT (no consumer in #1100, #1106, #1133, #1138 — admin cancel UI / job watchdog are not on any open roadmap issue). 5. CapacityReport.withSpareForcedZero (single call site; inlined). 6. Duplicate MAX_FRAME constant (HubProperties.MAX_FRAME_SIZE_BYTES → FrameCodec.MAX_FRAME_BYTES). Comment surgery: - pi-runner.mjs / pi-mentor-runner.mjs: stripped TUM-GPU war-story anecdotes and 6-line "register provider DIRECTLY because..." preambles. - LlmProxyAuthShell javadoc: 12-line essay → 6-line summary. - PracticeDetectionResultParser: dropped 5 "Step N:" restating comments. - AgentJobExecutor: removed condition edit-history banner. - WorkerSessionRegistry, WorkerControlClient, AgentJobCancellationReason: similar trimming. Kept (against initial YAGNI verdict, evidence-driven): - WorkerDisconnectedEvent → #1138 lease release. - WorkerJwtVerifier interface → #1133 ArchUnit rule locks the abstraction boundary (library-portability seam, not over-abstraction). Also fixed: - Liquibase changelog header ("Five logical changes" → "Six"; the file has six changesets). All 2858 unit tests green. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
| } | ||
|
|
||
| /** JDK {@link WebSocket.Listener} that buffers inbound text into the dispatch queue. */ | ||
| private final class Listener implements WebSocket.Listener { |
The previous commit deleted provider-openai.ts / provider-anthropic.ts but
left server/agent-extensions/ pointing at them. CI failed with TS18003
("No inputs were found"). Removes:
- server/agent-extensions/ workspace (package.json + tsconfig.json)
- pnpm-workspace.yaml entry + @earendil-works/* publicHoistPattern (only
the deleted .ts files needed it)
- ci-quality-gates.yml typecheck step
- webapp/Dockerfile COPY line for the workspace manifest
The Pi SDK is still resolved at runtime via /opt/pi-sdk/node_modules in the
agent-pi image (npm install --prefix). Runtime typecheck of the runner
scripts happens via the pi-mentor-runner.spec.mjs smoke test, which stays.
Root cause: the consolidated changelog bumped agent_config.llm_base_url from
VARCHAR(512) to VARCHAR(2048), but the ERD was generated against my local
dev DB that still had the column at 512 (legacy migration applied before
the bump). CI applies the merged changelog to a fresh Postgres and gets
VARCHAR(2048), so it correctly flagged the schema drift.
Reproduced the CI environment locally:
docker run -d --name erd-fresh-pg -p 5499:5432 postgres:16 …
mvn -pl server liquibase:update -Dpostgres.port=5499
node --import tsx scripts/generate-mermaid-erd.ts \
jdbc:postgresql://localhost:5499/hephaestus root root \
docs/contributor/erd/schema.mmd
Diff is one line: VARCHAR(512) → VARCHAR(2048).
The consolidated Liquibase changelog bumped the DB column from VARCHAR(512)
to VARCHAR(2048), but the JPA entity was left at length=512. The
draft-changelog CI gate (Hibernate-vs-DB diff) flagged this as schema drift
and wanted to generate a migration *shrinking* the DB back to 512.
Root-cause fix: align the entity with the migration. Reproduced the CI
check locally with a fresh Postgres on :5500 + CI=true:
docker run -d --name draft-fresh-pg -p 5500:5432 postgres:16
mvn -pl server liquibase:update -Dpostgres.port=5500
CI=true POSTGRES_PORT=5500 scripts/db-utils.sh draft-changelog
Before fix: changelog_new.xml generated with a modifyDataType shrinking
llm_base_url to varchar(512). After fix: "No database changes detected".
cancelInFlight calls jobRepository.findByStatus(RUNNING) globally with no worker-scoping. The existing javadoc claims safety from "JetStream WorkQueue + maxAckPending ensures only this worker holds RUNNING messages" — true for a 1-worker deployment, false the moment worker replicas scale to >=2: a drain on worker A will mass-cancel worker B's RUNNING jobs. Adds a TODO referencing #1138 (dispatcher claim loop). The real fix — scoping the query to jobs claimed by THIS worker — requires the claimed_by_worker_id column the dispatcher epic adds, so it belongs in that PR, not this one. Comment-only change; no runtime behaviour moves.
This was referenced May 23, 2026
Collaborator
Author
|
🎉 This PR is included in version 0.73.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
FelixTJDietrich
added a commit
that referenced
this pull request
Jul 21, 2026
Eleven review findings verified against the code; every real one fixed: - A dedicated worker couldn't serve the LLM proxy at all: the worker profile disabled the HTTP connector (server.port -1, pre-existing from #1302) while the sandbox targets the worker's own /internal/llm. Workers now bind a real port; a boot-matrix test proves the prod,worker property set wires the proxy chain. - Fair polling: candidates whose config is already at its concurrency cap are excluded in SQL, so capped configs can't starve younger runnable jobs; new partial indexes back the queued scan and the running-count check. - Retry integrity: pool-rejected claims requeue via a self-fenced CAS that does NOT burn retry_count (bounded retries + poll backoff); orphan requeue and terminal transitions are fenced on worker_id with the retry cap enforced in SQL, so racing sweepers can't double-requeue or steal a reclaimed run. - Drain honesty: shutdown joins the poll thread before awaiting in-flight work (closing the claim-after-drain window), and drain timeout now requeues fenced jobs as documented instead of terminally cancelling them. - Config hygiene: the worker profile drops dead agent.nats/worker-LLM properties (which also false-positived the startup warner every boot) and enables the agent by default; the warner covers all four retired NATS variables; poll and heartbeat durations are validated at startup. Unit 5391/5391, architecture 217/217, integration 1160/1160, liquibase update+rollback validated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important
Tested topology: 1 app-pod + 1 worker-pod, live-validated end-to-end against the GPU/ASE LLM gateway (8 LLM calls, finding produced, job COMPLETED).
N-app multi-pod routing for cross-pod sessions is out of scope — deferred to #1100 dispatcher work.
Closes #1099. Closes #1098.
What this is
The worker-runtime substrate plus its control channel. The same JAR boots as a slim worker pod (
SPRING_PROFILES_ACTIVE=prod,worker), dials the application server overwss://…/api/workers/connect, reports its capacity every 20 s, drains gracefully onSIGTERM, and surfaces its state at/actuator/healthand viaworker_*Prometheus metrics.The two epics (#1099 worker substrate and #1098 control channel) were bundled because the WSS-over-TLS-443 choice was driven by the BYO / self-hosted requirement — workers behind enterprise MITM proxies that block non-443 traffic. Shipping the substrate without its transport would leave a stub. The bundled scope is recorded in ADR 0009 (
docs/decisions/0009-worker-runtime-substrate-wss-control-channel.md).Operational impact — what to know before merging
1. Default monolith deployments are unchanged
HEPHAESTUS_RUNTIME_{SERVER,WORKER,WEBHOOK}_ENABLEDall default totrue(matchIfMissing=true). After this PR:HEPHAESTUS_HUB_URLenv →WorkerControlClientis gated off by@ConditionalOnExpressionthat rejects empty strings. No self-dial, no reconnect loop.WorkerCapacityStateper job (review claimed / released).HEPHAESTUS_LLM_BASE_URLandHEPHAESTUS_LLM_API_KEYare set on the JVM. The default monolith deploy sets neither.One log line you'll see on monolith restart:
WARN: Generated ephemeral signing key (kid=default). Stable key required for prod.SetHEPHAESTUS_WORKER_HUB_SIGNING_KEYto silence it.2. Multi-pod behavior
HEPHAESTUS_HUB_URL→ registered on ONE app-pod onlyWorkerSessionRegistrysendsForceReconnect+GOING_AWAYbefore Tomcat stopsMentor interactive session frames + the hub-side bridge were explicitly removed in the YAGNI cuts (see "What changed" below). They live in the mentor-runtime epic #1106 (issues #1157/#1159/#1161), which will rebuild them on the
MentorChatServiceSPI.3. New HTTP endpoints
POST /api/workers/exchange— registration-token → 1 h RS256 JWT. Closed by default (returns 404 unlesshephaestus.worker.hub.token.registration-tokenis set). Per-IP throttle: 10 fails / minute.GET/Upgrade /api/workers/connect— WSS handshake. JWT inAuthorization: Bearer ….WorkerJwtHandshakeInterceptorrejects on missing/invalid/revoked token.WorkerHellotimeout (default 10 s) closes half-open sessions.4. New env vars
App-pod
HEPHAESTUS_WORKER_HUB_TOKEN_REGISTRATION_TOKENHEPHAESTUS_WORKER_HUB_TOKEN_SIGNING_KEYWorker pod
HEPHAESTUS_HUB_URLWorkerControlClientnot wired; substrate quiescentHEPHAESTUS_WORKER_REGISTRATION_TOKENHEPHAESTUS_WORKER_CAPACITY_{REVIEW,MENTOR}_MAXautomax(1, cpu - 1)/max(1, cpu / 2)HEPHAESTUS_WORKER_DRAIN_TIMEOUT5m0= immediate cancel5. New Prometheus metrics (
worker_*namespace)worker_capacity_{total,in_flight,spare}{type=review|mentor}worker_control_frames_{sent,received,dropped},worker_control_reconnects,worker_control_channel_connectedworker_drain_activeworker_hub_sessions_active,worker_hub_handshake_completed,worker_hub_hello_timeout,worker_hub_binary_refused,worker_hub_draining_signalled,worker_hub_frame_{decode,dispatch}_failed,worker_hub_transport_errorsworker_token_exchange{outcome,reason},worker_jwt_verify{outcome,reason}worker_heartbeats_{sent,failed}6. Database (forward-only, idempotent)
A single consolidated changelog
1779520390544_worker_runtime_substrate.xmlships six preconditioned changesets (eachonFail=MARK_RANfor safe replays across worktrees):agent_job.cancellation_reason VARCHAR(32)— nullable drain audit columnworker_token_denylist(jti PK, revoked_at, expires_at)+ index onexpires_atpractice.criteriafrompractice.descriptionwhere emptypractice.descriptionpractice.criteriaNOT NULLagent_config.llm_base_url VARCHAR(2048)— per-workspace LLM gateway URL7. New Spring Security filter chains
@Order(HIGHEST_PRECEDENCE)workerHubSecurityFilterChain— matches/api/workers/**,/actuator/health{,/**},/actuator/info,/gitlab,/github. Auth at controller / handshake layer. Other actuator paths (loggers, metrics, prometheus) fall through to the OAuth2 chain unchanged.@ConditionalOnBean(JwtDecoder.class)resourceServerSecurityFilterChain— existing user-facing chain. Now conditional so worker-only pods can boot without Keycloak.@ConditionalOnMissingBean(name="resourceServerSecurityFilterChain")lockdownSecurityFilterChain— deny-all fallback when no OAuth2.8. New Docker Compose service
application-workerblock indocker/compose.app.yaml— opt-in, only starts if your compose orchestration includes the service.stop_grace_period: 6maligned with the worker's 5 m default drain budget.What changed during review
Late in review, three principal-engineer subagents audited every speculative surface against the actual backlog issues (#1100, #1106, #1133, #1138, #1159, #1161). The audit drove ~1,150 LoC of ruthless deletes (commit
8b76e01ef):Deleted (no near-term consumer in any open issue):
MentorSessionBridge,HubSessionRegistry,BridgedMentorController) + worker-side dispatcher / runner / session protocol records → epic epic: worker runtime substrate — slim profile, capacity heartbeat, drain, health #1099 explicitly carved this out as deferred; epic: move mentor sessions onto workers with Last-Event-ID resume #1106 will rebuild onMentorChatServiceSPI, not on this bridge.WorkerControlPublishersingle-impl interface → epic: move mentor sessions onto workers with Last-Event-ID resume #1106 explicitly defers NATS cross-pod fanout (the only second-transport that could justify the seam).WorkerConnectedEvent→ zero listeners across all open issues.AgentJobCancellationReason.USER/.TIMEOUT→ no admin-cancel UI or job watchdog on any roadmap issue.CapacityReport.withSpareForcedZero→ single call site, inlined.MAX_FRAMEconstant.Kept (evidence-backed):
WorkerDisconnectedEvent→ consumed by feat(server): dispatcher claim loop with SKIP LOCKED, lease reclaim, disconnect release #1138 lease-release next sprint.WorkerJwtVerifierinterface → feat(server): WorkerJwtVerifier SPI, /api/workers/exchange endpoint, JTI denylist #1133 ArchUnit rule locks the abstraction boundary as a library-portability seam.Test plan
Automated (CI)
WorkerCapacityReporterTest,WorkerDrainCoordinatorTest,WorkerControlChannelHealthIndicatorTest,WorkerCapacityStateTest,FrameCodecRoundTripTest,WorkerSessionRegistryTest,WorkerControlChannelIntegrationTestWorkerJwtTest,WorkerJwtIssuerTest,WorkerJwtHandshakeIntegrationTest,WorkerTokenExchangeIntegrationTest,JavaJwtWorkerJwtVerifierTestRuntimeRoleBoundaryTestenforces@ConditionalOnPropertygates on every runtime-scoped class;CodeQualityTestenforcesObjectProvidercycle-breaker allowlistManually live-validated
End-to-end practice review against the TUM GPU LLM gateway (
https://gpu.ase.cit.tum.de) producing a real finding:status=COMPLETED, exit_code=0, llm_total_calls=8, input_tokens=27595, output_tokens=666, schemaVersion=3/opt/pi-sdk/node_modulesinstall (no content-hashed paths)Out of scope (deferred)
kid-keyed ring with rotation via redeploy + overlappingkeys[]. Endpoint is straightforward when needed.Watch list for merge day
HEPHAESTUS_WORKER_HUB_SIGNING_KEYis set, every app-pod restart regenerates the worker-signing keypair, invalidating any in-flight worker JWT. Workers reconnect within seconds.hephaestus.worker.drain.timeout=5m(default),spring.lifecycle.timeout-per-shutdown-phase=6m, composestop_grace_period: 6m. If you tune one, tune all three.AgentJobExecutor@ConditionalOnExpressiongate: combineshephaestus.agent.nats.enabledANDhephaestus.runtime.worker.enabledbecause Spring honors only one@ConditionalOnPropertyper element. Default monolith mode is unchanged (both flags default-true).🤖 Generated with Claude Code