feat(server): Java webhook runtime role; restarts no longer drop events - #1300
Merged
FelixTJDietrich merged 6 commits intoMay 21, 2026
Merged
Conversation
Reimplements `webhook-ingest/` (Node/Hono/jnats-js) in Java as a third runtime role next to `server` and `worker`. Same JAR, two containers: the existing `application-server` and a new `webhook-server` activated with `SPRING_PROFILES_ACTIVE=prod,webhook`. Restart-independent — a deploy of the app server no longer drops webhook traffic. Closes #1110. Why --- - One image, one observability stack, one deploy pipeline. - ADR 0005's named revisit trigger ("Java webhook-ingest becomes a third runtime role") fires here. Captured in new ADR 0008. - Push events on GitHub/GitLab are not manually redeliverable; the receiver must be restart-independent from the app server. Two containers from one image satisfies that without splitting the build. Substrate --------- - New `gitprovider.webhook` sub-package (Modulith sub-package, no own `@ApplicationModule`): `HmacVerifier` (SHA-256, constant-time, length-tolerant), `GitLabTokenVerifier`, `GitLabSubjectBuilder`, `GitHubSubjectBuilder`, `DedupIdResolver`, `JetStreamPublisher` (Resilience4j retry + Phaser drain + Micrometer counters), `StreamBootstrap` (addStream-or-getStreamInfo; fail-closed on conflict), `WebhookHealthIndicator`, `WebhookGracefulShutdown` (SmartLifecycle phase = `WebServerGracefulShutdownLifecycle.SMART_LIFECYCLE_PHASE - 1024`), `WebhookPayloadSizeFilter` (411 on missing Content-Length, 413 over cap; pre-Spring buffer). - `WebhookProperties` moved to `core/webhook/` and exposed via `@NamedInterface("webhook")`. `toString()` redacts `secret`. - `application-webhook.yml` profile overlay flips `server.enabled=false`, `worker.enabled=false`, `webhook.enabled=true`, `liquibase.enabled=false`. - `RuntimeRole` adds `WEBHOOK_PROPERTY`. `@EnableScheduling` extracted into `ServerSchedulingConfig` (gated on `SERVER_PROPERTY`). `NatsConsumerService` and `WorkspaceStartupListener` gated on `SERVER_PROPERTY`. Five workspace services use `ObjectProvider<NatsConsumerService>` so the webhook profile boots without re-introducing the consumer. Parity with the Node port ------------------------- - Byte-equal subject builder, dedup-id resolver, group fallback chain, GitLab `/-/` URL parsing, event-name normalisation. - `HexFormat.of()` everywhere; `Integer.toHexString` / `String.format("%02x")` banned in the package via ArchUnit. - `Locale.ROOT` enforced at every case-fold site (Turkish-dotted-i bug) via ArchUnit. - 196 captured fixtures power `SubjectGrammarRoundTripTest`; producer and consumer (`NatsConsumerService.buildSubjectPrefix`) verified byte-aligned. Operational ----------- - `webhook.publish{outcome}` and `webhook.publish.retry` (publisher); `webhook.rejected{provider, reason}` (controllers + filter) — counters cached per-reason via `ConcurrentHashMap.computeIfAbsent` to avoid registry churn. - Graceful shutdown ordering: HTTP drains first, then `Phaser` drains in-flight publishes up to `hephaestus.webhook.shutdown.drain-timeout`, then NATS closes. - Docker Compose: new `webhook-server` service with `stop_grace_period: 40s` (covers HTTP graceful + drain budget). `webhook-ingest` block removed; Traefik `/webhooks` strip-prefix routed to the new container. Architecture safeguards (ArchUnit) ---------------------------------- - `enableSchedulingLivesOnlyOnServerSchedulingConfig` — direct or meta-annotated `@EnableScheduling` outside `ServerSchedulingConfig` fails CI. - `webhookPackageIsIsolatedFromServerWorkerConcerns` — `gitprovider.webhook` can't depend on `workspace`, `leaderboard`, or `agent`. - `RuntimeRoleBoundaryTest.expectedRuntimeGatesArePresent` — `WebhookConfiguration`, both controllers, `ServerSchedulingConfig`, `NatsConsumerService`, `WorkspaceStartupListener` carry the right `@ConditionalOnProperty`. - `HexEncodingArchTest`, `LocaleSafetyArchTest` — the two parity landmines pinned at compile time. Node decommission ----------------- - `webhook-ingest/` directory deleted (~3.5k LOC, 30 files). - `pnpm-workspace.yaml`, `package.json` (17 scripts), `pnpm-lock.yaml`, CI workflows (`cicd`, `ci-tests`, `ci-quality-gates`, `ci-docker-build`, `release`, `deploy-prod`, `deploy-staging`), `CODEOWNERS`, `labeler.yml`, `renovate.json`, `project.code-workspace`, `webapp/Dockerfile`, `commitlint.config.mjs`, contributor docs, the bug-report issue template, opencode/Claude prompts — all purged. Validation ---------- - 2981 unit/architecture tests pass; 9-test `WebhookSmokeIT` (MockMvc + recording publisher, byte-equality + every error path) passes. - Manual runtime validation on a fresh stack: both processes boot clean, restart-independence proven (151/151 webhook posts returned 200 across a window that killed the app-server JVM mid-flight), live GitHub + GitLab webhooks delivered end-to-end through a cloudflared tunnel, NATS subjects and dedup IDs byte-equal.
Contributor
📝 WalkthroughWalkthroughMigrates webhook ingestion from the removed Node service to Java: adds Spring controllers, publisher, stream bootstrap, filters, runtime-role gating/config, updates CI/CD and Docker wiring, and revises docs and tests accordingly. ChangesWebhook receiver integration and Node decommission
Sequence Diagram(s)sequenceDiagram
participant ComponentA
participant ComponentB
ComponentA->>ComponentB: observable interaction
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Contributor
📚 Documentation Preview
|
![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: 11
🤖 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 @.github/prompts/fix-ci.prompt.md:
- Around line 61-63: Update the "Build failure" row (the table cell containing
the literal text "Build failure") so it covers both webapp and server-side
compile/build failures: instruct contributors to verify the webapp with `pnpm
run build:webapp` and also to run the server build (e.g., `cd server && ./mvnw
-DskipTests=false package` or the project's canonical server build command) and
to check for missing exports or compilation errors in both codebases; ensure the
row includes both commands and a brief note to run the server test/build command
shown elsewhere in the file (e.g., the App server tests row commands) so
server-side CI failures are diagnosed correctly.
In @.github/prompts/land-pr.prompt.md:
- Line 25: The lockfile change detection line that currently reads
"`package.json` OR `package-lock.json` OR `.node-version` → webapp changed"
misses pnpm's lockfile; update the mapping in the prompt to include
`pnpm-lock.yaml` (e.g., add "OR `pnpm-lock.yaml`") so dependency-only changes
with pnpm are detected; ensure you update the exact string in the prompt (the
line containing `package-lock.json`/`.node-version`) so the changed-components
logic recognizes pnpm lockfile updates.
In @.opencode/commands/land-pr.md:
- Around line 24-26: Update the changed-component detection rule that currently
checks for package-lock.json to also consider pnpm-lock.yaml so pnpm-only
lockfile changes trigger the webapp validation; specifically, in the logic/text
that lists "package.json OR package-lock.json OR .node-version → webapp" add
pnpm-lock.yaml alongside package-lock.json (ensure any tooling/parser that reads
this rule is updated to check both lockfiles).
In `@docker/preview/compose.shared-infra.yaml`:
- Line 64: The sample webhook URL "https://example.com/webhooks:8080" reverses
the port and path; update the example in the Coolify comment to use the correct
URL syntax (place the port immediately after the host and the path after that),
e.g. "https://example.com:8080/webhooks", so replace the string
"https://example.com/webhooks:8080" with the corrected format in the
compose.shared-infra.yaml comment.
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/github/GitHubWebhookController.java`:
- Around line 80-90: The branches in GitHubWebhookController that currently
return ad-hoc maps via error("...") should instead construct and return RFC-7807
ProblemDetail objects (e.g., ProblemDetail.forStatus(HttpStatus.UNAUTHORIZED) or
ProblemDetail.forStatus(HttpStatus.BAD_REQUEST)), populate type/title/detail
(include the error code like "missing-secret" / "invalid-signature" /
"missing-event-header" and contextual info such as deliveryId), and return them
in the ResponseEntity body; update the branches around the HmacVerifier.verify
call and the eventType checks (and the similar branches at the other noted
locations) to build and return ProblemDetail rather than using error(...),
keeping the existing log/rejected(...) calls intact.
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/JetStreamPublisher.java`:
- Around line 57-63: The catch in publish(...) currently swallows
InterruptedException thrown by publishOnce(...) and wraps it in
PublishFailedException without restoring the thread's interrupt status; update
publish(...) to handle InterruptedException separately (either by adding a catch
(InterruptedException ie) { Thread.currentThread().interrupt();
failureCounter.increment(); throw new PublishFailedException(..., ie); } before
the generic catch or by detecting instanceof InterruptedException in the
existing catch and calling Thread.currentThread().interrupt() before wrapping),
keeping the failureCounter.increment() and preserving the original exception as
the cause.
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/StreamBootstrap.java`:
- Around line 142-144: The warning currently skips cases where live <= 0
(sentinel values), hiding real drift; update warnIfDiffersLong to log whenever
the observed value differs from expected by changing the guard from "if (live >
0 && live != expected)" to simply check "if (live != expected)" so non-positive
sentinel values (e.g., unlimited) are also compared and will produce a warning;
keep the existing log message and parameter order in log.warn to preserve
context.
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/workspace/GitLabWorkspaceInitializationService.java`:
- Around line 212-214: When NATS is enabled (natsProperties.enabled()) the code
currently calls natsConsumerService.ifAvailable(...) and silently does nothing
if the consumer bean is absent; change both call sites (the updateScopeConsumer
call around workspace creation and the other occurrence at lines ~798-800) to
detect the absence and surface an error: if natsProperties.enabled() is true and
natsConsumerService.isAvailable()/presence check returns false (or if
ifAvailable(...) callback is not invoked), log an error with context and/or
throw an IllegalStateException indicating the required NATS consumer bean is
missing so webhook consumption failure is visible and fails fast instead of
being a silent no-op (refer to natsConsumerService.ifAvailable(...),
natsProperties.enabled(), updateScopeConsumer).
In
`@server/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/web/WebhookPayloadSizeFilterTest.java`:
- Around line 113-115: The test WebhookPayloadSizeFilterTest currently allocates
a 1_000_000_000-byte array via new byte[1_000_000_000] when creating the
MockHttpServletRequest to exercise bypass behavior; replace this heavyweight
allocation by setting a small byte[] (e.g. a few KB) or by mocking/overriding
the request content length (use MockHttpServletRequest#setContentLength or a
mocked HttpServletRequest that returns a large getContentLengthLong) so the test
asserts the same bypass logic for request.setContent/getContentLength without
actually allocating 1GB of memory.
In
`@server/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/WebhookSmokeIT.java`:
- Line 67: Tests use Files.readAllBytes(Paths.get("src/test/resources/..."))
which depends on working directory; change WebhookSmokeIT to load fixtures from
the classpath instead (e.g., use
WebhookSmokeIT.class.getResourceAsStream("/gitlab/push.json") or
Thread.currentThread().getContextClassLoader().getResourceAsStream("gitlab/push.json")),
read the stream into a byte[] and replace the Files.readAllBytes calls (the
occurrences that produce the local variable body) with this classpath-based
loading for all affected spots.
🪄 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: b6cb45a9-9153-4586-8340-60694509bdf5
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (118)
.github/CODEOWNERS.github/ISSUE_TEMPLATE/bug_report.yml.github/labeler.yml.github/prompts/fix-ci.prompt.md.github/prompts/land-pr.prompt.md.github/workflows/ci-docker-build.yml.github/workflows/ci-quality-gates.yml.github/workflows/ci-tests.yml.github/workflows/cicd.yml.github/workflows/deploy-prod.yml.github/workflows/deploy-staging.yml.github/workflows/release.yml.opencode/commands/fix-ci.md.opencode/commands/land-pr.mdAGENTS.mdcommitlint.config.mjsdocker/compose.app.yamldocker/compose.core.yamldocker/preview/.env.exampledocker/preview/README.mddocker/preview/compose.shared-infra.yamldocs/contributor/ai-code-review.mdxdocs/contributor/ci-cd.mdxdocs/contributor/coding-guidelines.mdxdocs/contributor/local-development.mdxdocs/contributor/overview.mdxdocs/contributor/system-design.mdxdocs/decisions/0001-flat-top-level-layout.mddocs/decisions/0005-two-role-runtime-via-conditional-on-property.mddocs/decisions/0008-webhook-runtime-role.mdpackage.jsonpnpm-workspace.yamlproject.code-workspacerenovate.jsonscripts/jean-setup.shserver/pom.xmlserver/src/main/java/de/tum/cit/aet/hephaestus/Application.javaserver/src/main/java/de/tum/cit/aet/hephaestus/SecurityConfig.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/RuntimeRole.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/ServerSchedulingConfig.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/runtime/package-info.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/webhook/WebhookProperties.javaserver/src/main/java/de/tum/cit/aet/hephaestus/core/webhook/package-info.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/sync/NatsConsumerService.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/DedupIdResolver.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/GitLabSubjectBuilder.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/GitLabTokenVerifier.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/HmacVerifier.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/JetStreamPublisher.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/PublishRequest.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/StreamBootstrap.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/WebhookConfiguration.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/WebhookGracefulShutdown.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/WebhookHealthIndicator.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/github/GitHubSubjectBuilder.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/github/GitHubWebhookController.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/package-info.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/web/GitLabWebhookController.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/web/WebhookPayloadSizeFilter.javaserver/src/main/java/de/tum/cit/aet/hephaestus/leaderboard/LeaderboardTaskScheduler.javaserver/src/main/java/de/tum/cit/aet/hephaestus/workspace/GitLabWebhookService.javaserver/src/main/java/de/tum/cit/aet/hephaestus/workspace/GitLabWorkspaceInitializationService.javaserver/src/main/java/de/tum/cit/aet/hephaestus/workspace/WebhookProperties.javaserver/src/main/java/de/tum/cit/aet/hephaestus/workspace/WorkspaceActivationService.javaserver/src/main/java/de/tum/cit/aet/hephaestus/workspace/WorkspaceInstallationService.javaserver/src/main/java/de/tum/cit/aet/hephaestus/workspace/WorkspaceLifecycleService.javaserver/src/main/java/de/tum/cit/aet/hephaestus/workspace/WorkspaceRepositoryMonitorService.javaserver/src/main/java/de/tum/cit/aet/hephaestus/workspace/WorkspaceStartupListener.javaserver/src/main/resources/application-webhook.ymlserver/src/main/resources/application.ymlserver/src/test/java/de/tum/cit/aet/hephaestus/architecture/CodeQualityTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/architecture/HexEncodingArchTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/architecture/LocaleSafetyArchTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/architecture/RuntimeRoleBoundaryTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/sync/NatsSubjectBuilderTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/sync/SubjectGrammarRoundTripTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/DedupIdResolverTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/GitHubSubjectBuilderTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/GitLabSubjectBuilderTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/GitLabTokenVerifierTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/HmacVerifierTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/JetStreamPublisherRetryTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/StreamBootstrapTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/WebhookHealthIndicatorTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/WebhookSmokeIT.javaserver/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/web/WebhookPayloadSizeFilterTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/workspace/GitLabWebhookServiceTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/workspace/GitLabWorkspaceInitializationServiceTest.javawebapp/Dockerfilewebhook-ingest/.dockerignorewebhook-ingest/.env.examplewebhook-ingest/.env.testwebhook-ingest/.gitignorewebhook-ingest/AGENTS.mdwebhook-ingest/Dockerfilewebhook-ingest/README.mdwebhook-ingest/biome.jsonwebhook-ingest/package.jsonwebhook-ingest/src/app.tswebhook-ingest/src/crypto/verify.tswebhook-ingest/src/env.tswebhook-ingest/src/index.tswebhook-ingest/src/logger.tswebhook-ingest/src/nats/client.tswebhook-ingest/src/routes/github.tswebhook-ingest/src/routes/gitlab.tswebhook-ingest/src/routes/health.tswebhook-ingest/src/utils/dedupe.tswebhook-ingest/src/utils/gitlab-subject.tswebhook-ingest/test/crypto/verify.test.tswebhook-ingest/test/routes/github.test.tswebhook-ingest/test/routes/gitlab.test.tswebhook-ingest/test/routes/health.test.tswebhook-ingest/test/utils/dedupe.test.tswebhook-ingest/test/utils/gitlab-subject.test.tswebhook-ingest/tsconfig.jsonwebhook-ingest/tsconfig.test.jsonwebhook-ingest/vitest.config.ts
💤 Files with no reviewable changes (37)
- webapp/Dockerfile
- webhook-ingest/AGENTS.md
- webhook-ingest/src/utils/dedupe.ts
- webhook-ingest/package.json
- webhook-ingest/test/utils/dedupe.test.ts
- webhook-ingest/tsconfig.test.json
- webhook-ingest/test/routes/gitlab.test.ts
- webhook-ingest/biome.json
- webhook-ingest/.env.test
- webhook-ingest/vitest.config.ts
- webhook-ingest/src/logger.ts
- webhook-ingest/src/app.ts
- scripts/jean-setup.sh
- webhook-ingest/Dockerfile
- webhook-ingest/test/routes/health.test.ts
- webhook-ingest/src/routes/github.ts
- webhook-ingest/test/utils/gitlab-subject.test.ts
- renovate.json
- webhook-ingest/.env.example
- webhook-ingest/src/routes/gitlab.ts
- webhook-ingest/src/crypto/verify.ts
- .github/labeler.yml
- webhook-ingest/tsconfig.json
- webhook-ingest/test/routes/github.test.ts
- webhook-ingest/.gitignore
- webhook-ingest/src/env.ts
- webhook-ingest/.dockerignore
- webhook-ingest/README.md
- webhook-ingest/src/routes/health.ts
- webhook-ingest/src/index.ts
- project.code-workspace
- webhook-ingest/test/crypto/verify.test.ts
- server/src/main/java/de/tum/cit/aet/hephaestus/workspace/WebhookProperties.java
- webhook-ingest/src/nats/client.ts
- webhook-ingest/src/utils/gitlab-subject.ts
- .github/workflows/ci-tests.yml
- .github/workflows/ci-quality-gates.yml
Comment on lines
+61
to
63
| | 4 | Build failure | Compilation errors, missing exports | Fix imports/exports, verify with `pnpm run build:webapp` | | ||
| | 5 | Webapp tests | "FAIL" in webapp test output | Fix test or source, verify with `pnpm run test:webapp` | | ||
| | 5 | App server tests | Maven test failures, assertion errors | Fix test or source, verify with `cd server && ./mvnw test -Dsurefire.includedGroups="unit" -Dmaven.test.skip=false -T 2C --batch-mode -q` | |
Contributor
There was a problem hiding this comment.
Build-failure guidance is scoped too narrowly.
The “Build failure” row currently validates only webapp builds. For server-side compile/build failures, this sends contributors down the wrong path.
Suggested adjustment
-| 4 | Build failure | Compilation errors, missing exports | Fix imports/exports, verify with `pnpm run build:webapp` |
+| 4 | Build failure | Compilation errors, missing exports | Fix imports/exports, verify with `pnpm run build:webapp` (webapp) or `cd server && ./mvnw -DskipTests compile --batch-mode -q` (app-server) |📝 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
| | 4 | Build failure | Compilation errors, missing exports | Fix imports/exports, verify with `pnpm run build:webapp` | | |
| | 5 | Webapp tests | "FAIL" in webapp test output | Fix test or source, verify with `pnpm run test:webapp` | | |
| | 5 | App server tests | Maven test failures, assertion errors | Fix test or source, verify with `cd server && ./mvnw test -Dsurefire.includedGroups="unit" -Dmaven.test.skip=false -T 2C --batch-mode -q` | | |
| | 4 | Build failure | Compilation errors, missing exports | Fix imports/exports, verify with `pnpm run build:webapp` (webapp) or `cd server && ./mvnw -DskipTests compile --batch-mode -q` (app-server) | | |
| | 5 | Webapp tests | "FAIL" in webapp test output | Fix test or source, verify with `pnpm run test:webapp` | | |
| | 5 | App server tests | Maven test failures, assertion errors | Fix test or source, verify with `cd server && ./mvnw test -Dsurefire.includedGroups="unit" -Dmaven.test.skip=false -T 2C --batch-mode -q` | |
🤖 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 @.github/prompts/fix-ci.prompt.md around lines 61 - 63, Update the "Build
failure" row (the table cell containing the literal text "Build failure") so it
covers both webapp and server-side compile/build failures: instruct contributors
to verify the webapp with `pnpm run build:webapp` and also to run the server
build (e.g., `cd server && ./mvnw -DskipTests=false package` or the project's
canonical server build command) and to check for missing exports or compilation
errors in both codebases; ensure the row includes both commands and a brief note
to run the server test/build command shown elsewhere in the file (e.g., the App
server tests row commands) so server-side CI failures are diagnosed correctly.
| - `webhook-ingest/**` → webhook changed | ||
| - `package.json` OR `package-lock.json` OR `.node-version` → webapp + webhook changed | ||
| - `server/**` OR `scripts/db-utils.sh` → app-server changed (includes webhook receiver since ADR 0008) | ||
| - `package.json` OR `package-lock.json` OR `.node-version` → webapp changed |
Contributor
There was a problem hiding this comment.
Lockfile change detection misses pnpm lockfile updates.
This mapping tracks package-lock.json but not pnpm-lock.yaml. Dependency-only changes in pnpm can be missed by the “changed components” logic in this prompt.
Suggested adjustment
-- `package.json` OR `package-lock.json` OR `.node-version` → webapp changed
+- `package.json` OR `pnpm-lock.yaml` OR `.node-version` → webapp changed📝 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
| - `package.json` OR `package-lock.json` OR `.node-version` → webapp changed | |
| - `package.json` OR `pnpm-lock.yaml` OR `.node-version` → webapp changed |
🤖 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 @.github/prompts/land-pr.prompt.md at line 25, The lockfile change detection
line that currently reads "`package.json` OR `package-lock.json` OR
`.node-version` → webapp changed" misses pnpm's lockfile; update the mapping in
the prompt to include `pnpm-lock.yaml` (e.g., add "OR `pnpm-lock.yaml`") so
dependency-only changes with pnpm are detected; ensure you update the exact
string in the prompt (the line containing `package-lock.json`/`.node-version`)
so the changed-components logic recognizes pnpm lockfile updates.
Comment on lines
+24
to
26
| - `server/**` OR `scripts/db-utils.sh` → app-server changed (includes webhook receiver since ADR 0008) | ||
| - `package.json` OR `package-lock.json` OR `.node-version` → webapp changed | ||
| - `docs/**` → docs-only (skip all validation if nothing else changed) |
Contributor
There was a problem hiding this comment.
Include pnpm-lock.yaml in changed-component detection.
Line 25 still keys dependency-change detection to package-lock.json; this can miss pnpm lockfile-only changes and skip required webapp validation for this flow.
Suggested patch
-- `package.json` OR `package-lock.json` OR `.node-version` → webapp changed
+- `package.json` OR `pnpm-lock.yaml` OR `.node-version` → webapp changed📝 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
| - `server/**` OR `scripts/db-utils.sh` → app-server changed (includes webhook receiver since ADR 0008) | |
| - `package.json` OR `package-lock.json` OR `.node-version` → webapp changed | |
| - `docs/**` → docs-only (skip all validation if nothing else changed) | |
| - `package.json` OR `package-lock.json` OR `pnpm-lock.yaml` OR `.node-version` → webapp changed |
🤖 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 @.opencode/commands/land-pr.md around lines 24 - 26, Update the
changed-component detection rule that currently checks for package-lock.json to
also consider pnpm-lock.yaml so pnpm-only lockfile changes trigger the webapp
validation; specifically, in the logic/text that lists "package.json OR
package-lock.json OR .node-version → webapp" add pnpm-lock.yaml alongside
package-lock.json (ensure any tooling/parser that reads this rule is updated to
check both lockfiles).
| # but with SPRING_PROFILES_ACTIVE=prod,webhook so only gitprovider.webhook beans load. | ||
| # Restart-independent from application-server (push events on GitHub/GitLab are | ||
| # not manually redeliverable). See ADR 0008. | ||
| # Configure domain in Coolify UI: https://example.com/webhooks:8080 |
Contributor
There was a problem hiding this comment.
Fix the webhook domain example format.
Line 64 uses https://example.com/webhooks:8080, but this reverses port/path order and can cause misconfiguration in Coolify docs.
Suggested patch
- # Configure domain in Coolify UI: https://example.com/webhooks:8080
+ # Configure domain in Coolify UI: https://example.com:8080/webhooks📝 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
| # Configure domain in Coolify UI: https://example.com/webhooks:8080 | |
| # Configure domain in Coolify UI: https://example.com:8080/webhooks |
🤖 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 `@docker/preview/compose.shared-infra.yaml` at line 64, The sample webhook URL
"https://example.com/webhooks:8080" reverses the port and path; update the
example in the Coolify comment to use the correct URL syntax (place the port
immediately after the host and the path after that), e.g.
"https://example.com:8080/webhooks", so replace the string
"https://example.com/webhooks:8080" with the corrected format in the
compose.shared-infra.yaml comment.
Comment on lines
+80
to
+90
| return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body(error("missing-secret")); | ||
| } | ||
| if (!HmacVerifier.verify(signature, secret, body)) { | ||
| log.warn("GitHub webhook rejected: invalid signature (deliveryId={})", deliveryId); | ||
| rejected("invalid-signature"); | ||
| return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body(error("invalid-signature")); | ||
| } | ||
| if (eventType == null || eventType.isBlank()) { | ||
| rejected("missing-event-header"); | ||
| return ResponseEntity.status(HttpStatus.BAD_REQUEST).body(error("missing-event-header")); | ||
| } |
Contributor
There was a problem hiding this comment.
Use RFC-7807 ProblemDetail for webhook error responses.
These branches return ad-hoc { "error": ... } maps instead of standardized ProblemDetail, which breaks the controller error contract.
As per coding guidelines server/src/main/java/**/*Controller.java: “every controller must return RFC-7807 ProblemDetail responses via @RestControllerAdvice”.
Also applies to: 99-101, 125-126, 140-141
🤖 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/webhook/github/GitHubWebhookController.java`
around lines 80 - 90, The branches in GitHubWebhookController that currently
return ad-hoc maps via error("...") should instead construct and return RFC-7807
ProblemDetail objects (e.g., ProblemDetail.forStatus(HttpStatus.UNAUTHORIZED) or
ProblemDetail.forStatus(HttpStatus.BAD_REQUEST)), populate type/title/detail
(include the error code like "missing-secret" / "invalid-signature" /
"missing-event-header" and contextual info such as deliveryId), and return them
in the ResponseEntity body; update the branches around the HmacVerifier.verify
call and the eventType checks (and the similar branches at the other noted
locations) to build and return ProblemDetail rather than using error(...),
keeping the existing log/rejected(...) calls intact.
Comment on lines
+83
to
+89
| return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body(error("missing-secret")); | ||
| } | ||
| if (!GitLabTokenVerifier.verify(token, secret)) { | ||
| log.warn("GitLab webhook rejected: invalid token"); | ||
| rejected("invalid-token"); | ||
| return ResponseEntity.status(HttpStatus.UNAUTHORIZED).body(error("invalid-token")); | ||
| } |
Contributor
There was a problem hiding this comment.
Return standardized ProblemDetail errors instead of map payloads.
The controller currently emits custom { "error": ... } responses on failure paths; this should be aligned to RFC-7807 ProblemDetail through the centralized exception flow.
As per coding guidelines server/src/main/java/**/*Controller.java: “every controller must return RFC-7807 ProblemDetail responses via @RestControllerAdvice”.
Also applies to: 95-97, 125-126, 140-141
Comment on lines
212
to
214
| if (created > 0 && natsProperties.enabled()) { | ||
| natsConsumerService.updateScopeConsumer(workspace.getId()); | ||
| natsConsumerService.ifAvailable(svc -> svc.updateScopeConsumer(workspace.getId())); | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid silent no-op when NATS is enabled but consumer bean is absent.
Both call sites now silently skip consumer operations under ifAvailable(...). If natsProperties.enabled() is true and the bean is missing, webhook consumption can stall without visibility.
Suggested hardening
- if (created > 0 && natsProperties.enabled()) {
- natsConsumerService.ifAvailable(svc -> svc.updateScopeConsumer(workspace.getId()));
- }
+ if (created > 0 && natsProperties.enabled()) {
+ NatsConsumerService svc = natsConsumerService.getIfAvailable();
+ if (svc == null) {
+ log.warn("NATS enabled but consumer bean unavailable: workspaceId={}", workspace.getId());
+ } else {
+ svc.updateScopeConsumer(workspace.getId());
+ }
+ }- if (natsProperties.enabled()) {
- natsConsumerService.ifAvailable(svc -> svc.startConsumingScope(workspace.getId()));
- }
+ if (natsProperties.enabled()) {
+ NatsConsumerService svc = natsConsumerService.getIfAvailable();
+ if (svc == null) {
+ log.warn("NATS enabled but consumer bean unavailable: workspaceId={}", workspace.getId());
+ } else {
+ svc.startConsumingScope(workspace.getId());
+ }
+ }Also applies to: 798-800
🤖 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/workspace/GitLabWorkspaceInitializationService.java`
around lines 212 - 214, When NATS is enabled (natsProperties.enabled()) the code
currently calls natsConsumerService.ifAvailable(...) and silently does nothing
if the consumer bean is absent; change both call sites (the updateScopeConsumer
call around workspace creation and the other occurrence at lines ~798-800) to
detect the absence and surface an error: if natsProperties.enabled() is true and
natsConsumerService.isAvailable()/presence check returns false (or if
ifAvailable(...) callback is not invoked), log an error with context and/or
throw an IllegalStateException indicating the required NATS consumer bean is
missing so webhook consumption failure is visible and fails fast instead of
being a silent no-op (refer to natsConsumerService.ifAvailable(...),
natsProperties.enabled(), updateScopeConsumer).
|
|
||
| @Test | ||
| void gitlabPushPublishesByteEqualBody() throws Exception { | ||
| byte[] body = Files.readAllBytes(Paths.get("src/test/resources/gitlab/push.json")); |
Contributor
There was a problem hiding this comment.
Use classpath resource loading instead of cwd-relative file paths.
Paths.get("src/test/resources/...") makes these tests brittle across runners. On Line 67 (and the same pattern at Lines 90, 106, 134, 204), prefer loading fixtures from the classpath.
Suggested diff
-import java.nio.file.Files;
-import java.nio.file.Paths;
+import java.io.InputStream;
...
- byte[] body = Files.readAllBytes(Paths.get("src/test/resources/gitlab/push.json"));
+ byte[] body = resourceBytes("/gitlab/push.json");
...
- byte[] body = Files.readAllBytes(Paths.get("src/test/resources/github/push.json"));
+ byte[] body = resourceBytes("/github/push.json");
...
+ private static byte[] resourceBytes(String path) throws Exception {
+ try (InputStream in = WebhookSmokeIT.class.getResourceAsStream(path)) {
+ if (in == null) {
+ throw new IllegalStateException("Missing test resource: " + path);
+ }
+ return in.readAllBytes();
+ }
+ }As per coding guidelines, tests should be repeatable with consistent setup/data and maintainable as execution environments vary.
Also applies to: 90-90, 106-106, 134-134, 204-204
🤖 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/gitprovider/webhook/WebhookSmokeIT.java`
at line 67, Tests use Files.readAllBytes(Paths.get("src/test/resources/..."))
which depends on working directory; change WebhookSmokeIT to load fixtures from
the classpath instead (e.g., use
WebhookSmokeIT.class.getResourceAsStream("/gitlab/push.json") or
Thread.currentThread().getContextClassLoader().getResourceAsStream("gitlab/push.json")),
read the stream into a byte[] and replace the Files.readAllBytes calls (the
occurrences that produce the local variable body) with this classpath-based
loading for all affected spots.
…mPublisher) Per-controller `@ConditionalOnProperty(WEBHOOK_PROPERTY, matchIfMissing=true)` let the controllers load whenever the env var wasn't set — even in profiles that legitimately can't satisfy their dependencies (test/specs/AOT build, all with `sync.nats.enabled=false`). `NatsConfig.Connection` doesn't load, so `WebhookConfiguration` doesn't load (it's `@ConditionalOnBean(Connection)`), so `JetStreamPublisher` doesn't exist — but the controllers still tried to wire it. Result: `UnsatisfiedDependencyException` breaking the integration suite, OpenAPI generation, and the arm64 buildpack image. Switch the controllers to `@ConditionalOnBean(JetStreamPublisher.class)` so they auto-disappear together with their dependency. Drop their entries from `RuntimeRoleBoundaryTest.EXPECTED_GATES` — the implicit chain (WebhookConfiguration property gate → JetStreamPublisher @bean → controllers) is now single-anchored and the arch test on `WebhookConfiguration` still locks in the runtime-role invariant.
The Java webhook receiver shipped with comments, ADR text, and agent-skill commands that explained the new code as a "port of the prior Node service" or invoked deleted commands like `pnpm run test:webhook-ingest`. Contributors shouldn't have to learn about a deleted service to read the current code. Production Java - Strip "byte-equal port of webhook-ingest/...", "mirrors the prior Node service", "mirrors TS `||` truthiness", and "Node's timingSafeEqual" from GitLabSubjectBuilder, GitHubSubjectBuilder, StreamBootstrap, HmacVerifier. - Reword "webhook-ingested" / "by the webhook-ingest layer" / "matching webhook-ingest's gitlab-subject.ts" in NatsConsumerService and the GitLab event-name handlers + DTOs. - Drop the redundant package-info paragraph that duplicated WebhookConfiguration's class Javadoc. - Drop bullet-restating-the-method-name Javadocs on `streamFor`, `awaitInFlight`, `normalizeEventName`, and the PHASE constant. - Drop the ADR-0005 hedge in WebhookConfiguration's Javadoc; `matchIfMissing` on the annotation is self-documenting. - Trim the JaCoCo "parity-landmine" rule comment to what it actually does. Tests - Strip "Port of webhook-ingest/test/..." Javadoc from DedupIdResolverTest, HmacVerifierTest, GitHubSubjectBuilderTest, GitLabSubjectBuilderTest. - Reword WebhookSmokeIT's "critical byte-equality" sentence as "preserves the raw request body unchanged". - Delete the dead @beforeeach in WebhookSmokeIT (recorder is a per-instance field; JUnit's default lifecycle is per-method, so the list is fresh). - Collapse seven near-duplicate null/blank validation tests in NatsSubjectBuilderTest into two parameterised `@CsvSource` tests. Docs - Rewrite ADR 0008 Java-first: lead with the operational fact (push events not redeliverable) instead of the Node-service narrative. Drop the "Considered options: keep Node service" dead branch and the line-counts "we removed N LOC of TS" paragraph (that belongs in the PR description). - Rewrite ADR 0001 title and body (was "Java + TypeScript deployables"); the "Update" stanza is folded into the body. - Patch ADR 0005's "webhook-ingest stays TypeScript" line and the Revisit trigger / Update block so they read as Java-first prose. - Add ADR 0008 to docs/decisions/README.md. - Fix docs/admin/production-setup.mdx — "Webhook ingest (Hono/TypeScript)" was flat-out wrong; the production stack is webhook-server (same image). - AGENTS.md, docs/contributor/coding-guidelines.mdx, and docs/contributor/local-development.mdx: drop "ported byte-equal from the prior Node service" / "1:1 ported Vitest cases" / "TypeScript services". CI / agent scaffolding - Drop "webhooks" scope from .github/PULL_REQUEST_TEMPLATE.md. - Drop the obsolete "webhook-ingest receiver" CODEOWNERS comment. - .claude/skills/{fix-ci,land-pr}/SKILL.md: remove `pnpm run build:webhook-ingest` and `pnpm run test:webhook-ingest` commands (the scripts no longer exist; future CI fix loops would fail). Renumber sections. - .github/prompts/land-pr.prompt.md, .opencode/commands/land-pr.md: drop the "No standalone TS services left" parenthetical and the now-empty "Build Affected Services" section. Renumber. application-webhook.yml - Replace the `# ⇒ ServerSchedulingConfig, ...` arrow-comment block that restated RuntimeRole.java Javadoc verbatim with a tighter header that points at ADR 0008.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| "gitlab, group//project, consecutive slash", | ||
| } | ||
| ) | ||
| void rejectsEmptySegments(String stream, String input, String description) { |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.claude/skills/land-pr/SKILL.md (1)
138-138:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winScope lists differ between landing-PR documentation files.
This file lists scopes
webapp,server,ai,webhooks,docswhile.opencode/commands/land-pr.mdline 124 lists onlywebapp,server,docs. The inconsistency may confuse contributors about which scopes are valid.🔧 Recommendation
Align both files to use the same scope list. If
aiandwebhooksare valid scopes, add them to.opencode/commands/land-pr.md. Otherwise, remove them from this file to match the other.🤖 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 @.claude/skills/land-pr/SKILL.md at line 138, The scope list in SKILL.md ("Service: `webapp`, `server`, `ai`, `webhooks`, `docs`") is inconsistent with the list in .opencode/commands/land-pr.md (which currently has only "`webapp`, `server`, `docs`"); decide which set of scopes is authoritative and make both files match: either add `ai` and `webhooks` to the scope list in .opencode/commands/land-pr.md, or remove `ai` and `webhooks` from SKILL.md so both show the same scopes; update the scope string in the two files (search for the literal scope lists) to keep them identical and run a quick repo search for other occurrences to ensure consistency.server/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/github/GitHubWebhookController.java (1)
94-100:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInvalid-JSON branch silently swallows the parse exception.
Unlike the other rejection paths (missing-secret, invalid-signature, publish-failed) which log with context, this one bumps the
invalid-jsoncounter without logging anything — making it hard to diagnose malformed payloads in production. IncludedeliveryId(and ideallyeventType) plus the exception message.Suggested fix
JsonNode payload; try { payload = objectMapper.readTree(body); } catch (Exception e) { + log.warn("GitHub webhook rejected: invalid JSON (deliveryId={} event={}): {}", deliveryId, eventType, e.getMessage()); rejected("invalid-json"); return ResponseEntity.status(HttpStatus.BAD_REQUEST).body(error("invalid-json")); }As per coding guidelines: "Include context in log messages (workspace, user, request ID) to aid troubleshooting."
🤖 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/webhook/github/GitHubWebhookController.java` around lines 94 - 100, The invalid-JSON catch in GitHubWebhookController swallows the parse exception; update the catch for objectMapper.readTree(body) to log the exception and context before calling rejected("invalid-json") and returning the 400 response: include deliveryId and eventType (read from headers or variables already available in the method), the exception message (e.getMessage()), and any relevant request/context identifiers in the processLogger or logger call used elsewhere in this class so the log mirrors other rejection branches (e.g., the same pattern used for "missing-secret" / "invalid-signature") prior to returning the error response.
♻️ Duplicate comments (4)
.opencode/commands/land-pr.md (1)
25-25:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate lockfile detection to match project's package manager.
The repository uses pnpm (per learnings), but this line still checks for
package-lock.json. This can miss pnpm-only lockfile changes and skip required webapp validation.📦 Suggested fix
-- `package.json` OR `package-lock.json` OR `.node-version` → webapp changed +- `package.json` OR `pnpm-lock.yaml` OR `.node-version` → webapp changed🤖 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 @.opencode/commands/land-pr.md at line 25, The checklist that detects webapp changes currently looks for `package.json` OR `package-lock.json` OR `.node-version` but the repo uses pnpm; update the condition in the land-pr checklist text to include pnpm's lockfile (e.g., add `pnpm-lock.yaml`) or replace `package-lock.json` with `pnpm-lock.yaml` so pnpm-only updates trigger webapp validation; ensure the displayed line `package.json OR package-lock.json OR .node-version → webapp changed` is updated accordingly to reference `pnpm-lock.yaml` (and optionally keep both lockfile names if you want to support multiple package managers)..claude/skills/land-pr/SKILL.md (1)
39-39:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate lockfile detection to match project's package manager.
This line still references
package-lock.json, but the repository uses pnpm (per learnings). Pnpm-only lockfile changes would be missed, potentially skipping required webapp validation.📦 Suggested fix
-- `package.json` OR `package-lock.json` OR `.node-version` → webapp changed +- `package.json` OR `pnpm-lock.yaml` OR `.node-version` → webapp changed🤖 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 @.claude/skills/land-pr/SKILL.md at line 39, Update the lockfile detection line in SKILL.md to reference pnpm's lockfile instead of npm's: replace the existing "package-lock.json" token with "pnpm-lock.yaml" so the checklist reads something like "`package.json` OR `pnpm-lock.yaml` OR `.node-version` → webapp changed"; ensure the wording still matches the repository's package manager detection logic and test that pnpm-only changes are now considered by the webapp validation instructions..github/prompts/land-pr.prompt.md (1)
25-25:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWebapp change detection still misses
pnpm-lock.yaml.This mapping still only watches
package-lock.json, but the repo uses pnpm — dependency-only changes viapnpm-lock.yamlwon't be classified as a webapp change, and the rest of this prompt (component-specific test selection in step 6) silently skips webapp tests.Suggested adjustment
-- `package.json` OR `package-lock.json` OR `.node-version` → webapp changed +- `package.json` OR `pnpm-lock.yaml` OR `.node-version` → webapp changedBased on learnings, the repo uses pnpm with
pnpm-lock.yamland pnpm workspaces.🤖 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 @.github/prompts/land-pr.prompt.md at line 25, The webapp change detection mapping that currently looks for the string "`package.json` OR `package-lock.json` OR `.node-version` → webapp changed`" must be updated to include pnpm artifacts used by this repo; add "`pnpm-lock.yaml`" (and optionally "`pnpm-workspace.yaml`" if present) to that mapping so dependency-only changes via pnpm are classified as webapp changes; update the line containing that mapping in .github/prompts/land-pr.prompt.md (search for the exact mapping string) to include these filenames.server/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/github/GitHubWebhookController.java (1)
79-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWebhook error responses still use ad-hoc
{ "error": ... }maps.All five rejection/error branches return
Map<String,String>bodies instead of standardizedProblemDetail, breaking the controller error contract.Suggested shape
- private static Map<String, String> error(String code) { - return Map.of("error", code); - } + private static ProblemDetail error(HttpStatus status, String code, String detail) { + ProblemDetail problem = ProblemDetail.forStatusAndDetail(status, detail); + problem.setTitle(code); + return problem; + }…and update each
ResponseEntity.status(...).body(error(...))call accordingly, then change the return type toResponseEntity<?>or split success/error endpoints.As per coding guidelines for
server/src/main/java/**/*Controller.java: "every controller must return RFC-7807ProblemDetailresponses 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/gitprovider/webhook/github/GitHubWebhookController.java` around lines 79 - 124, The controller returns ad-hoc Map error bodies; update GitHubWebhookController to return RFC-7807 ProblemDetail for all error branches (replace ResponseEntity.status(...).body(error("...")) in the missing-secret, invalid-signature, missing-event-header, invalid-json and publish-failed branches with ResponseEntity.status(...).body(ProblemDetail) using the standard ProblemDetail factory or your app's helper that creates ProblemDetail instances), adjust the method signature from ResponseEntity<String> to ResponseEntity<?> (or split success/error responses) and ensure the existing error(...) helper (if present) is replaced or overloaded to produce ProblemDetail objects so the `@RestControllerAdvice` handlers and controller contract remain consistent.
🧹 Nitpick comments (2)
server/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/StreamBootstrap.java (1)
29-29: 💤 Low valueConsider using
@Slf4jLombok annotation.The guidelines recommend using Lombok's
@Slf4jannotation instead of manualLoggerFactory.getLogger. This would remove the manual logger declaration and align with the project's Lombok conventions.♻️ Proposed refactor
+import lombok.extern.slf4j.Slf4j; + +@Slf4j public class StreamBootstrap { - private static final Logger log = LoggerFactory.getLogger(StreamBootstrap.class); private static final String[] STREAMS = { "gitlab", "github" };As per coding guidelines, use Lombok annotations:
@Slf4jfor logging instead of manual logger initialization.🤖 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/webhook/StreamBootstrap.java` at line 29, Replace the manual logger declaration in StreamBootstrap by adding Lombok's `@Slf4j` to the class and removing the private static final Logger log = LoggerFactory.getLogger(StreamBootstrap.class) line; update imports accordingly (remove org.slf4j.Logger/LoggerFactory and add lombok.extern.slf4j.Slf4j) so the class uses the generated 'log' field provided by Lombok instead of the hand-written logger.server/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/web/GitLabWebhookController.java (1)
41-41: 💤 Low valueConsider using
@Slf4jLombok annotation.The guidelines recommend using Lombok's
@Slf4jannotation instead of manualLoggerFactory.getLogger. This would remove the manual logger declaration and align with the project's Lombok conventions.♻️ Proposed refactor
+import lombok.extern.slf4j.Slf4j; + +@Slf4j `@RestController` `@ConditionalOnBean`(JetStreamPublisher.class) `@WorkspaceAgnostic`( "Webhook reception is provider-keyed (group/project). Workspace context is resolved downstream by the sync consumer." ) public class GitLabWebhookController { - private static final Logger log = LoggerFactory.getLogger(GitLabWebhookController.class); - private static final String HEADER_TOKEN = "X-Gitlab-Token";As per coding guidelines, use Lombok annotations:
@Slf4jfor logging instead of manual logger initialization.🤖 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/webhook/web/GitLabWebhookController.java` at line 41, Replace the manual logger declaration in GitLabWebhookController by adding Lombok's `@Slf4j` to the class: remove the private static final Logger log = LoggerFactory.getLogger(GitLabWebhookController.class); declaration, add the `@Slf4j` annotation to the GitLabWebhookController class, and remove unused imports for Logger and LoggerFactory so the class uses the Lombok-provided 'log' field instead.
🤖 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.
Outside diff comments:
In @.claude/skills/land-pr/SKILL.md:
- Line 138: The scope list in SKILL.md ("Service: `webapp`, `server`, `ai`,
`webhooks`, `docs`") is inconsistent with the list in
.opencode/commands/land-pr.md (which currently has only "`webapp`, `server`,
`docs`"); decide which set of scopes is authoritative and make both files match:
either add `ai` and `webhooks` to the scope list in
.opencode/commands/land-pr.md, or remove `ai` and `webhooks` from SKILL.md so
both show the same scopes; update the scope string in the two files (search for
the literal scope lists) to keep them identical and run a quick repo search for
other occurrences to ensure consistency.
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/github/GitHubWebhookController.java`:
- Around line 94-100: The invalid-JSON catch in GitHubWebhookController swallows
the parse exception; update the catch for objectMapper.readTree(body) to log the
exception and context before calling rejected("invalid-json") and returning the
400 response: include deliveryId and eventType (read from headers or variables
already available in the method), the exception message (e.getMessage()), and
any relevant request/context identifiers in the processLogger or logger call
used elsewhere in this class so the log mirrors other rejection branches (e.g.,
the same pattern used for "missing-secret" / "invalid-signature") prior to
returning the error response.
---
Duplicate comments:
In @.claude/skills/land-pr/SKILL.md:
- Line 39: Update the lockfile detection line in SKILL.md to reference pnpm's
lockfile instead of npm's: replace the existing "package-lock.json" token with
"pnpm-lock.yaml" so the checklist reads something like "`package.json` OR
`pnpm-lock.yaml` OR `.node-version` → webapp changed"; ensure the wording still
matches the repository's package manager detection logic and test that pnpm-only
changes are now considered by the webapp validation instructions.
In @.github/prompts/land-pr.prompt.md:
- Line 25: The webapp change detection mapping that currently looks for the
string "`package.json` OR `package-lock.json` OR `.node-version` → webapp
changed`" must be updated to include pnpm artifacts used by this repo; add
"`pnpm-lock.yaml`" (and optionally "`pnpm-workspace.yaml`" if present) to that
mapping so dependency-only changes via pnpm are classified as webapp changes;
update the line containing that mapping in .github/prompts/land-pr.prompt.md
(search for the exact mapping string) to include these filenames.
In @.opencode/commands/land-pr.md:
- Line 25: The checklist that detects webapp changes currently looks for
`package.json` OR `package-lock.json` OR `.node-version` but the repo uses pnpm;
update the condition in the land-pr checklist text to include pnpm's lockfile
(e.g., add `pnpm-lock.yaml`) or replace `package-lock.json` with
`pnpm-lock.yaml` so pnpm-only updates trigger webapp validation; ensure the
displayed line `package.json OR package-lock.json OR .node-version → webapp
changed` is updated accordingly to reference `pnpm-lock.yaml` (and optionally
keep both lockfile names if you want to support multiple package managers).
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/github/GitHubWebhookController.java`:
- Around line 79-124: The controller returns ad-hoc Map error bodies; update
GitHubWebhookController to return RFC-7807 ProblemDetail for all error branches
(replace ResponseEntity.status(...).body(error("...")) in the missing-secret,
invalid-signature, missing-event-header, invalid-json and publish-failed
branches with ResponseEntity.status(...).body(ProblemDetail) using the standard
ProblemDetail factory or your app's helper that creates ProblemDetail
instances), adjust the method signature from ResponseEntity<String> to
ResponseEntity<?> (or split success/error responses) and ensure the existing
error(...) helper (if present) is replaced or overloaded to produce
ProblemDetail objects so the `@RestControllerAdvice` handlers and controller
contract remain consistent.
---
Nitpick comments:
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/StreamBootstrap.java`:
- Line 29: Replace the manual logger declaration in StreamBootstrap by adding
Lombok's `@Slf4j` to the class and removing the private static final Logger log =
LoggerFactory.getLogger(StreamBootstrap.class) line; update imports accordingly
(remove org.slf4j.Logger/LoggerFactory and add lombok.extern.slf4j.Slf4j) so the
class uses the generated 'log' field provided by Lombok instead of the
hand-written logger.
In
`@server/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/web/GitLabWebhookController.java`:
- Line 41: Replace the manual logger declaration in GitLabWebhookController by
adding Lombok's `@Slf4j` to the class: remove the private static final Logger log
= LoggerFactory.getLogger(GitLabWebhookController.class); declaration, add the
`@Slf4j` annotation to the GitLabWebhookController class, and remove unused
imports for Logger and LoggerFactory so the class uses the Lombok-provided 'log'
field instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f533fe66-38ed-4f45-9b0d-a844fbb284c8
📒 Files selected for processing (43)
.claude/skills/fix-ci/SKILL.md.claude/skills/land-pr/SKILL.md.github/CODEOWNERS.github/PULL_REQUEST_TEMPLATE.md.github/prompts/land-pr.prompt.md.opencode/commands/land-pr.mdAGENTS.mddocs/admin/production-setup.mdxdocs/contributor/coding-guidelines.mdxdocs/contributor/local-development.mdxdocs/decisions/0001-flat-top-level-layout.mddocs/decisions/0005-two-role-runtime-via-conditional-on-property.mddocs/decisions/0008-webhook-runtime-role.mddocs/decisions/README.mdserver/pom.xmlserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/commit/CommitRepository.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/commit/github/CommitMetadataEnrichmentService.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/common/gitlab/GitLabEventType.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/organization/gitlab/GitLabMemberMessageHandler.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/organization/gitlab/dto/GitLabMemberEventDTO.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/repository/gitlab/GitLabProjectEventMessageHandler.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/repository/gitlab/dto/GitLabProjectEventDTO.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/sync/NatsConsumerService.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/team/gitlab/GitLabSubgroupMessageHandler.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/team/gitlab/dto/GitLabSubgroupEventDTO.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/GitLabSubjectBuilder.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/HmacVerifier.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/JetStreamPublisher.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/StreamBootstrap.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/WebhookConfiguration.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/WebhookGracefulShutdown.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/github/GitHubSubjectBuilder.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/github/GitHubWebhookController.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/package-info.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/web/GitLabWebhookController.javaserver/src/main/resources/application-webhook.ymlserver/src/test/java/de/tum/cit/aet/hephaestus/architecture/RuntimeRoleBoundaryTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/sync/NatsSubjectBuilderTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/DedupIdResolverTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/GitHubSubjectBuilderTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/GitLabSubjectBuilderTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/HmacVerifierTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/WebhookSmokeIT.java
💤 Files with no reviewable changes (4)
- server/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/WebhookGracefulShutdown.java
- server/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/GitHubSubjectBuilderTest.java
- server/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/HmacVerifierTest.java
- server/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/JetStreamPublisher.java
✅ Files skipped from review due to trivial changes (16)
- server/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/organization/gitlab/dto/GitLabMemberEventDTO.java
- server/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/repository/gitlab/dto/GitLabProjectEventDTO.java
- docs/admin/production-setup.mdx
- server/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/repository/gitlab/GitLabProjectEventMessageHandler.java
- server/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/commit/CommitRepository.java
- server/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/organization/gitlab/GitLabMemberMessageHandler.java
- server/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/team/gitlab/GitLabSubgroupMessageHandler.java
- docs/decisions/README.md
- server/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/team/gitlab/dto/GitLabSubgroupEventDTO.java
- server/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/commit/github/CommitMetadataEnrichmentService.java
- server/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/common/gitlab/GitLabEventType.java
- .github/CODEOWNERS
- .github/PULL_REQUEST_TEMPLATE.md
- docs/decisions/0005-two-role-runtime-via-conditional-on-property.md
- server/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/package-info.java
- docs/decisions/0008-webhook-runtime-role.md
…ness fixes
Round 3 review surfaced four real issues. Each fix below is one of them.
1. Resilience4j Retry would burn 5 attempts on an `IllegalArgumentException`
thrown by `JetStreamPublisher.streamFor()` (which only fires on a
programmer bug — unknown subject prefix — but every "retry storm"
counter increment skews the SLI). Add
`ignoreExceptions(IllegalArgumentException.class)` to the Retry config.
2. Both controllers were INFO-logging every successful publish
("Published GitLab webhook: subject=… event=…"). The publisher already
DEBUG-logs the same fact. At fleet scale ≈ 10 RPS that's ~864k lines
per pod per day with no extra signal. Demote both to DEBUG.
3. Provider sub-packages were asymmetric — `webhook/github/` held the
GitHub controller + subject builder, but the GitLab controller lived
in `webhook/web/` alongside `WebhookPayloadSizeFilter`, and the GitLab
subject builder + token verifier + the GitHub-only `HmacVerifier` all
sat naked in the root `webhook/` package. A newcomer looking for the
GitLab controller looked under `webhook.gitlab.*` first and was wrong.
New layout:
webhook/gitlab/ GitLabWebhookController, GitLabSubjectBuilder,
GitLabTokenVerifier
webhook/github/ GitHubWebhookController, GitHubSubjectBuilder,
HmacVerifier (HmacVerifier is GitHub-only, not shared)
webhook/web/ WebhookPayloadSizeFilter (cross-cutting HTTP layer)
webhook/ DedupIdResolver, JetStreamPublisher, StreamBootstrap,
PublishRequest, WebhookConfiguration,
WebhookGracefulShutdown, WebhookHealthIndicator
Tests moved to match. Stale `target/classes` from the move caused a
ConflictingBeanDefinition on first re-run; cleaned via `mvn clean`.
4. Polish: `WebhookProperties` had `@DurationUnit(...) @DefaultValue("9s")`
on every Duration field. The suffix on the default wins, so the unit
annotation is silent noise on every field. Drop the redundant
`@DurationUnit` annotation across all five Duration sites and the now-
unused imports. Dead `effectiveEvent != null` guard in
`GitLabWebhookController` (the prior assignment guarantees non-null)
reduced to `!effectiveEvent.isEmpty()`.
2986 unit/architecture + 11 integration tests pass.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/sync/SubjectGrammarRoundTripTest.java (1)
41-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Files.list(...)stream is never closed.
Files.list(dir)opens a directory handle that requires closing; here the returnedStream<Path>is handed straight to@MethodSourceand the caller never closes it. JUnit will iterate but won't invokeStream.close()reliably across runs, which can leak file descriptors on platforms with strict handle limits.♻️ Suggested fix
- private static Stream<Path> listJson(Path dir) throws IOException { - if (!Files.isDirectory(dir)) { - return Stream.empty(); - } - return Files.list(dir) - .filter(p -> p.getFileName().toString().endsWith(".json")) - .sorted(); - } + private static Stream<Path> listJson(Path dir) throws IOException { + if (!Files.isDirectory(dir)) { + return Stream.empty(); + } + try (Stream<Path> entries = Files.list(dir)) { + return entries + .filter(p -> p.getFileName().toString().endsWith(".json")) + .sorted() + .collect(java.util.stream.Collectors.toList()) + .stream(); + } + }🤖 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/gitprovider/sync/SubjectGrammarRoundTripTest.java` around lines 41 - 48, The Files.list stream opened in listJson is not closed; change listJson so it opens the directory stream in a try-with-resources, materializes the filtered+sorted paths into a List (e.g. List<Path> paths = s.filter(...).sorted().collect(Collectors.toList())) and then return paths.stream() (or return the List/Iterable if the test method source supports it) so the original Stream from Files.list is closed; update the method listJson(Path) to use try (Stream<Path> s = Files.list(dir)) { ... } and return the collected stream to avoid leaking directory handles.
🧹 Nitpick comments (2)
server/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/sync/SubjectGrammarRoundTripTest.java (2)
30-31: ⚡ Quick winCwd-relative fixture paths are brittle across runners.
Paths.get("src/test/resources/...")only resolves when the JVM is started from the module's working directory. Loading via classpath (getClass().getResource("/gitlab")→Path.of(url.toURI())) makes the test independent of the launching shell. This mirrors a previously flagged finding onWebhookSmokeITand applies here too.🤖 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/gitprovider/sync/SubjectGrammarRoundTripTest.java` around lines 30 - 31, Change the cwd-relative fixture paths in SubjectGrammarRoundTripTest to load resources from the classpath instead of using Paths.get("src/test/resources/..."); specifically replace the GITLAB_DIR and GITHUB_DIR initializations so they resolve via getClass().getResource("/gitlab") and getClass().getResource("/github") (or the class' ClassLoader), convert the returned URL to a URI and then to a Path (e.g., Path.of(url.toURI())), ensuring the test no longer depends on the JVM working directory.
52-65: ⚡ Quick winSilent skip via early-return can mask parity drift.
When
pathWithNamespacedoesn't contain/(Line 59), the parameterized test passes with zero assertions — the same is true for the GitHub variant at Lines 72-79. If a future change stripspath_with_namespacefrom all fixtures, every test would still "pass." Consider asserting at least once per invocation, orAssumptions.assumeTrue(...)so JUnit reports the skip explicitly and a smoke assertion catches "every fixture was skipped" via a non-parameterized companion test.- if (pathWithNamespace.contains("/")) { - String consumerPrefix = NatsConsumerService.buildSubjectPrefix("gitlab", pathWithNamespace); - assertThat(publisherSubject) - .as("publisher %s should start with consumer prefix '%s.'", fixture.getFileName(), consumerPrefix) - .startsWith(consumerPrefix + "."); - } + org.junit.jupiter.api.Assumptions.assumeTrue( + pathWithNamespace.contains("/"), + "no extractable namespace/project for " + fixture.getFileName() + ); + String consumerPrefix = NatsConsumerService.buildSubjectPrefix("gitlab", pathWithNamespace); + assertThat(publisherSubject) + .as("publisher %s should start with consumer prefix '%s.'", fixture.getFileName(), consumerPrefix) + .startsWith(consumerPrefix + ".");🤖 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/gitprovider/sync/SubjectGrammarRoundTripTest.java` around lines 52 - 65, The test method gitlabFixtureSubjectMatchesConsumerPrefix currently silently does nothing when pathWithNamespace doesn’t contain "/", which can mask regressions; change the logic to use JUnit assumptions or an explicit assertion instead of an early no-op: call org.junit.jupiter.api.Assumptions.assumeTrue(pathWithNamespace.contains("/"), "fixture %s has no namespace" with fixture.getFileName()) so skipped cases are reported, and/or add a fallback assertion that fails if all parameterized invocations are skipped (add a small non-parameterized companion test that asserts at least one fixture yields pathWithNamespace containing "/"); keep references to GitLabSubjectBuilder.build(payload) and NatsConsumerService.buildSubjectPrefix("gitlab", pathWithNamespace) unchanged.
🤖 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.
Outside diff comments:
In
`@server/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/sync/SubjectGrammarRoundTripTest.java`:
- Around line 41-48: The Files.list stream opened in listJson is not closed;
change listJson so it opens the directory stream in a try-with-resources,
materializes the filtered+sorted paths into a List (e.g. List<Path> paths =
s.filter(...).sorted().collect(Collectors.toList())) and then return
paths.stream() (or return the List/Iterable if the test method source supports
it) so the original Stream from Files.list is closed; update the method
listJson(Path) to use try (Stream<Path> s = Files.list(dir)) { ... } and return
the collected stream to avoid leaking directory handles.
---
Nitpick comments:
In
`@server/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/sync/SubjectGrammarRoundTripTest.java`:
- Around line 30-31: Change the cwd-relative fixture paths in
SubjectGrammarRoundTripTest to load resources from the classpath instead of
using Paths.get("src/test/resources/..."); specifically replace the GITLAB_DIR
and GITHUB_DIR initializations so they resolve via
getClass().getResource("/gitlab") and getClass().getResource("/github") (or the
class' ClassLoader), convert the returned URL to a URI and then to a Path (e.g.,
Path.of(url.toURI())), ensuring the test no longer depends on the JVM working
directory.
- Around line 52-65: The test method gitlabFixtureSubjectMatchesConsumerPrefix
currently silently does nothing when pathWithNamespace doesn’t contain "/",
which can mask regressions; change the logic to use JUnit assumptions or an
explicit assertion instead of an early no-op: call
org.junit.jupiter.api.Assumptions.assumeTrue(pathWithNamespace.contains("/"),
"fixture %s has no namespace" with fixture.getFileName()) so skipped cases are
reported, and/or add a fallback assertion that fails if all parameterized
invocations are skipped (add a small non-parameterized companion test that
asserts at least one fixture yields pathWithNamespace containing "/"); keep
references to GitLabSubjectBuilder.build(payload) and
NatsConsumerService.buildSubjectPrefix("gitlab", pathWithNamespace) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83aed7cf-0c9e-4e0e-a266-73f3e450a775
📒 Files selected for processing (14)
server/src/main/java/de/tum/cit/aet/hephaestus/core/webhook/WebhookProperties.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/WebhookConfiguration.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/github/GitHubWebhookController.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/github/HmacVerifier.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/gitlab/GitLabSubjectBuilder.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/gitlab/GitLabTokenVerifier.javaserver/src/main/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/gitlab/GitLabWebhookController.javaserver/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/sync/NatsSubjectBuilderTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/sync/SubjectGrammarRoundTripTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/WebhookSmokeIT.javaserver/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/github/GitHubSubjectBuilderTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/github/HmacVerifierTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/gitlab/GitLabSubjectBuilderTest.javaserver/src/test/java/de/tum/cit/aet/hephaestus/gitprovider/webhook/gitlab/GitLabTokenVerifierTest.java
shouldNotFilter() short-circuits before any body read, so the allocation bought zero coverage while creating heap-pressure flake risk on CI runners constrained to -Xmx2g. Removing the setContent() call also tightens the test to exactly what it asserts (path predicate, not body handling).
warnIfDiffersLong gated on `live > 0`, so a stream configured with maxMessages=-1 (unlimited retention) silently passed the drift check against a bounded expected value. Operators relying on the WARN to spot config divergence got nothing. Drop the guard; the contract is "log when live != expected" and -1 is a legitimate live value, not a sentinel for "unset".
FelixTJDietrich
changed the title
7 tasks
Collaborator
Author
|
🎉 This PR is included in version 0.72.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
FelixTJDietrich
added a commit
that referenced
this pull request
Jul 13, 2026
docker/preview/* was never updated for the native-auth cutover (#1317) or the Java webhook receiver (#1300, #1306), and had drifted far enough that no stack could boot. Previews are now opt-in: Coolify's all-or-nothing preview switch stays off, and the Preview / Coolify job refreshes only the previews that already exist. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 tasks
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.
Description
Webhook receivers (GitHub push, GitLab MR) are not redeliverable on demand, so any application-server restart could silently drop events. This PR moves the receiver into its own process so the app server can restart without taking the ingest path down with it.
Concretely: the Node
webhook-ingest/service is reimplemented as a third Spring runtime role (webhook) inside the existing JAR. Production now runs two containers from one image —application-server(default profile) andwebhook-server(prod,webhookoverlay) — sharing a NATS stream. Restart independence was validated under load: 151/151 POSTs returned 200 while the app-server JVM was killed mid-flight; the consumer caught up from JetStream with no gaps.Design and risks: ADR 0008. ADR 0001 and ADR 0005 updated accordingly.
Closes #1110.
What's in
gitprovider.webhookModulith package: HMAC-SHA256 verifier (MessageDigest.isEqual), GitLab token verifier, per-provider subject + dedup-id builders, JetStream publisher (Resilience4j retry withignoreExceptions(IllegalArgumentException),Phaserin-flight tracking),SmartLifecycledrain ordered before HTTP shutdown,WebhookPayloadSizeFilter(411 on missingContent-Length, 413 over cap).@ConditionalOnProperty(hephaestus.runtime.webhook.enabled, matchIfMissing=true);application-webhook.ymloverlay disablesserver/worker/liquibase.@EnableSchedulingextracted toServerSchedulingConfig; five workspace services switched toObjectProvider<NatsConsumerService>so the webhook profile boots clean.@EnableSchedulinguniqueness,webhookisolated fromworkspace/leaderboard/agent,HexFormat.of()-only,Locale.ROOT-only.webhook.publish{outcome},webhook.publish.retry,webhook.rejected{provider,reason}.webhook-serverCompose service (stop_grace_period: 40s), Traefik/webhooksstrip-prefix route.webhook-ingest/directory, its CI lanes, lockfile entry, CODEOWNERS entry, and renovate config deleted.How to test
Migration notes
/webhooks/github,/webhooks/gitlab).WEBHOOK_SECRETandWEBHOOK_EXTERNAL_URLunchanged.HEPHAESTUS_RUNTIME_WEBHOOK_ENABLED=falseon theapplication-servercontainer so receiver beans don't load there. Defaulttruepreserves single-processpnpm dev.Not in this PR (deferred)
ProblemDetailerror bodies — error shape is still ad-hoc text.@SpringBootTest(profiles=webhook)context-loads test — would have caught the three wiring bugs that ArchUnit now pins; needs a stubbedConnectionbean.ApplicationReadyEventpresence guard forNatsConsumerService— currently relies onObjectProviderlookup at call sites.