Skip to content

fix(gateway): derive the HTTP body limit and reject oversize with 413 - #131

Merged
cuioss-oliver merged 16 commits into
mainfrom
feature/plan-33-body-cap-derivation
Jul 30, 2026
Merged

fix(gateway): derive the HTTP body limit and reject oversize with 413#131
cuioss-oliver merged 16 commits into
mainfrom
feature/plan-33-body-cap-derivation

Conversation

@cuioss-oliver

@cuioss-oliver cuioss-oliver commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Summary

Derives the framework body-size limit from the configured security_filter anchors so
quarkus.http.limits.max-body-size is fail-closed-checked against the largest declared
max_body_bytes, making the gateway (not Vert.x) the component that rejects an oversized
body. A breach now surfaces as a CONTENT_TOO_LARGE(INPUT_VALIDATION, 413) RFC 9457
problem response instead of a bare framework 400/413 with no envelope. Adds a containerised
IT proving both the positive (accepted) and negative (gateway-rejected, envelope-discriminated)
cases, reconciles doc/configuration.adoc and doc/architecture.adoc, fixes the
TOPOLOGY_UPSTREAM benchmark-overlay no-op so the placeholder actually binds, and
re-enables the previously-quarantined uploadLarge benchmark goal now that its defect is
fixed and it runs against the intended static backend.

Changes

  • Body-limit derivation & 413 event contract

    • api-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducer.java — fail-closed boot check deriving/validating the framework body limit against configured anchor maxima
    • api-sheriff/src/main/java/de/cuioss/sheriff/gateway/events/EventType.java — new CONTENT_TOO_LARGE(INPUT_VALIDATION, 413) event
    • api-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStage.java, api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/DispatchStage.java — repoint the two body-cap raise sites at the new 413 event
    • api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapper.java — status-mapping follow-through for the new event
    • api-sheriff/src/main/resources/application.properties — derived/operator-set Quarkus limit configuration
    • Tests: ConfigProducerTest.java, ConfigFailFastTest.java, ThoroughChecksStageTest.java, DispatchStageTest.java, GrpcDispatchStageTest.java, GrpcStatusMapperTest.java, EventTypeTest.java
  • Documentation reconciliation

    • doc/configuration.adoc — the max_body_bytes row now states the declared cap is the effective one
    • doc/architecture.adoc — RFC 9457 error-contract table gains the new 413 row
  • Large-body regression IT

    • integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/LargeBodyIT.java — positive (above framework default, below declared cap → accepted) and negative (above declared cap → gateway-rejected, envelope-discriminated via content-type + category + null method) cases
    • integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BodyLimitActivationWiringTest.java — activation-wiring coverage
    • integration-tests/src/main/docker/nginx/body-limits.conf, integration-tests/docker-compose.yml — supporting IT topology
  • TOPOLOGY_UPSTREAM no-op fix (finding acf3e3)

    • integration-tests/src/main/docker/sheriff-config/topology.properties — placeholder now binds ${TOPOLOGY_UPSTREAM} instead of a bare literal, default keeps the IT stack on go-httpbin
    • integration-tests/docker-compose.benchmark.yml — corrected precedence-mechanism comments
  • Re-enable uploadLarge benchmark

    • benchmarks/pom.xmlskip.benchmark.upload.large now defaults to false
    • .github/workflows/benchmark.yml — moved out of the skipped table into the coverage expected list, updated count prose
    • benchmarks/README.adoc — removed stale "skipped" prose
  • Unrelated-surface follow-through swept in under pr_strategy: ride

    • integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieSessionIT.java, BffCookieStatelessnessIT.java, BffKeycloakLoginFlow.java, BffSessionMediationIT.java — in-plan test follow-up riding this PR per the compact pr_strategy decision

Test Plan

  • Quality gate passed (verify -Ppre-commit)
  • Full verify passed (verify)
  • Containerised LargeBodyIT positive/negative cases pass
  • Post-merge Performance Benchmark run confirms uploadLarge produces a summary document

Related Issues

None — no issue_url recorded for this plan.


Generated by plan-finalize skill

Summary by CodeRabbit

  • New Features

    • Added consistent request-body size handling that returns HTTP 413 for oversized bodies.
    • Mapped 413 to gRPC RESOURCE_EXHAUSTED for body-cap breaches.
    • Added fail-closed startup validation for framework body-size floor vs declared route caps.
  • Bug Fixes

    • Fixed body-cap breach classification so declared and streaming limits report the same 413/content-too-large behavior.
    • Prevented premature upstream rejections for large uploads and enabled the large-upload benchmark by default.
  • Documentation

    • Updated configuration, error-contract, architecture, and benchmark CI coverage guidance to reflect manifest-driven goal expectations and the revised 413/gRPC mappings.

cuioss-oliver and others added 7 commits July 29, 2026 19:38
Ship quarkus.http.limits.max-body-size=64M so a declared per-route max_body_bytes is reachable instead of being silently clipped by the Vert.x 10 MiB default, and add a fail-closed boot check in ConfigProducer that aborts when any declared cap exceeds the framework limit.

Both body-cap raise sites now throw the new CONTENT_TOO_LARGE (413) event instead of PARAMETER_LIMIT_EXCEEDED (400); enforcement predicates, thresholds and abort paths are unchanged. GrpcStatusMapper gains RESOURCE_EXHAUSTED (8) with the 413 arm and its class-javadoc row, so a gRPC body-cap breach no longer degrades to UNKNOWN.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UnvLgrWg6jzDc1hNemnvb7
topology.properties declared UPSTREAM as a bare literal, and EnvSecretResolver substitutes only explicit in-file placeholders — there is no TOPOLOGY_<ALIAS> precedence path — so the benchmark overlay's TOPOLOGY_UPSTREAM bound to nothing and every proxy aspect silently measured go-httpbin instead of nginx-static.

Declare UPSTREAM as ${TOPOLOGY_UPSTREAM:-http://go-httpbin:8080/anything}; the default keeps the IT stack on go-httpbin so the proxy ITs still observe its JSON echo. Two comments that asserted the non-existent mechanism are corrected.

Add an http-context nginx fragment for the static benchmark backend with lingering headroom sized for the 50 MB uploadLarge aspect, so the backend drains a large body rather than resetting it mid-transfer. Verified: nginx -t clean, and a 50 MB POST to location / returns 200 with the full body accepted.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UnvLgrWg6jzDc1hNemnvb7
Add BodyLimitActivationWiringTest, a fast no-Docker surefire guard that the committed descriptors activate the framework body floor — the unit-green / integration-red blind spot lesson 2026-07-25-15-001 names.

It asserts application.properties declares quarkus.http.limits.max-body-size, that the floor covers the largest declared max_body_bytes in every committed sheriff-config*/gateway.yaml (glob-discovered, so a new instance directory is covered automatically and an empty glob cannot pass vacuously), and that the IT container override exceeds LargeBodyIT's negative-case body size.

The third assertion asserts the QUARKUS_HTTP_LIMITS_MAX_BODY_SIZE override that the next deliverable adds; test-compile is green and the surefire run is covered at that deliverable's chain tail.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UnvLgrWg6jzDc1hNemnvb7

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @cuioss-oliver, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@cuioss-oliver, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 2 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 9f1c6440-5f53-470c-b2a7-88640a99909b

📥 Commits

Reviewing files that changed from the base of the PR and between de169eb and 500eec3.

📒 Files selected for processing (4)
  • .github/workflows/benchmark.yml
  • api-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.java
  • benchmarks/README.adoc
  • doc/adr/0023-The_declared_max_body_bytes_cap_is_the_effective_one_and_a_breach_is_the_gateways_413.adoc
📝 Walkthrough

Walkthrough

The change standardizes oversized request handling on HTTP 413, validates framework body limits during startup, adds large-body integration wiring and coverage, and enables manifest-driven benchmark reporting with uploadLarge running by default.

Changes

Body-size enforcement and error contract

Layer / File(s) Summary
413 body-size error contract
api-sheriff/src/main/java/..., api-sheriff/src/test/java/..., doc/*
Body-cap failures now emit CONTENT_TOO_LARGE with HTTP 413; gRPC maps it to RESOURCE_EXHAUSTED, with contract tests and documentation updated.
Framework body-limit startup validation
api-sheriff/src/main/java/de/.../ConfigProducer.java, api-sheriff/src/main/resources/application.properties, api-sheriff/src/test/java/.../quarkus/*
The framework body ceiling is configured and compared with declared anchor and route caps; startup rejects caps above that ceiling and tests cover the boundary.

Large-body integration and benchmark coverage

Layer / File(s) Summary
Large-body integration wiring and validation
integration-tests/docker-compose*, integration-tests/src/main/docker/*, integration-tests/src/test/java/.../integration/*
Upstream and framework limits are raised, Nginx drains uploads, topology supports overrides, and integration tests verify body-limit wiring and 413 rejection behavior.
Manifest-driven benchmark coverage
benchmarks/pom.xml, benchmarks/scripts/*, benchmarks/README.adoc, .github/workflows/benchmark.yml
uploadLarge runs by default; Maven generates an execution manifest, and CI derives expected and non-executing goals from it for coverage reporting.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the core gateway change: deriving the body limit and returning 413 for oversize requests.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@cuioss-review-bot

Copy link
Copy Markdown

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 No security concerns identified
⚡ No major issues detected

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (1)
api-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.java (1)

73-90: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Prevent the error-contract table from drifting from EventType.

The @CsvSource names a manually maintained subset of constants from api-sheriff/src/main/java/de/cuioss/sheriff/gateway/events/EventType.java. A future request-time failure added there can be omitted from this table while the suite still passes; adding the Line [81] row does not make coverage exhaustive. Derive the source or assert that the table’s keys equal every EventType with an HTTP mapping.

As per path instructions, hardcoded lists that mirror enum constants must be derived or guarded against drift; the authoritative definition is EventType.

🤖 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
`@api-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.java`
around lines 73 - 90, Update the parameterized test around the `@CsvSource` table
to derive its cases from EventType, or add an assertion that the table keys
exactly match every EventType constant with an HTTP mapping. Keep the existing
status-code and category assertions, but ensure newly added mapped EventType
values cannot be omitted without failing the test.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/benchmark.yml:
- Line 148: The benchmark coverage workflow must stop maintaining the hard-coded
expected list and instead consume a build-generated manifest produced from the
resolved Maven benchmark executions, including each benchmark’s enabled or
intentionally skipped state, property, and reason; update the coverage reporting
logic around the expected table and the Maven benchmark configuration that
generates the manifest. In benchmarks/README.adoc lines 199-204, retain the
documented skipped-goal reporting claim only if this manifest-driven behavior is
implemented; otherwise revise the documentation to state that suppressed goals
are not represented in the CI coverage table.

In `@doc/configuration.adoc`:
- Around line 944-949: Update the max_body_bytes documentation to make the
application/problem+json envelope conditional on the request reaching gateway
enforcement. Reconcile the related framework-floor and chunked-request
statements so they no longer promise the gateway envelope when the framework or
transport limit rejects the request first, while preserving the documented 413
behavior and enforcement order.

In
`@integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/LargeBodyIT.java`:
- Around line 155-165: Add an assertion in the LargeBodyIT response-validation
block to require the response status to equal HTTP 413 (CONTENT_TOO_LARGE),
while preserving the existing RFC 9457 envelope and upstream-rejection
assertions.

---

Outside diff comments:
In
`@api-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.java`:
- Around line 73-90: Update the parameterized test around the `@CsvSource` table
to derive its cases from EventType, or add an assertion that the table keys
exactly match every EventType constant with an HTTP mapping. Keep the existing
status-code and category assertions, but ensure newly added mapped EventType
values cannot be omitted without failing the test.
🪄 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: Repository: cuioss/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: d7dda21b-d0ea-4a73-b0ea-fb89835ccb9b

📥 Commits

Reviewing files that changed from the base of the PR and between 7c1bf0b and f9112fa.

📒 Files selected for processing (28)
  • .github/workflows/benchmark.yml
  • api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/DispatchStage.java
  • api-sheriff/src/main/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapper.java
  • api-sheriff/src/main/java/de/cuioss/sheriff/gateway/events/EventType.java
  • api-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStage.java
  • api-sheriff/src/main/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducer.java
  • api-sheriff/src/main/resources/application.properties
  • api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/DispatchStageTest.java
  • api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcDispatchStageTest.java
  • api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapperTest.java
  • api-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.java
  • api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStageTest.java
  • api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ConfigFailFastTest.java
  • api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducerTest.java
  • benchmarks/README.adoc
  • benchmarks/pom.xml
  • doc/architecture.adoc
  • doc/configuration.adoc
  • integration-tests/docker-compose.benchmark.yml
  • integration-tests/docker-compose.yml
  • integration-tests/src/main/docker/nginx/body-limits.conf
  • integration-tests/src/main/docker/sheriff-config/topology.properties
  • integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieSessionIT.java
  • integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffCookieStatelessnessIT.java
  • integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffKeycloakLoginFlow.java
  • integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BffSessionMediationIT.java
  • integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/BodyLimitActivationWiringTest.java
  • integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/LargeBodyIT.java

Comment thread .github/workflows/benchmark.yml Outdated
Comment thread doc/configuration.adoc
JsonPath.get is generic (<T> T get(String)). Inside String.valueOf(...) javac
resolves against the most specific applicable overload, String.valueOf(char[]),
inferring T = char[]; the JSON value is a String, so the negative case failed at
runtime with "class java.lang.String cannot be cast to class [C". The adjacent
assertions escaped it only because they infer Object.

Switch the type and title reads to JsonPath.getString and hoist the value into a
local, so the assertion message and the assertion share one already-typed read.

Verified against the containerised stack: LargeBodyIT runs 2/2 green.

Co-Authored-By: Claude <noreply@anthropic.com>
@cuioss-oliver

Copy link
Copy Markdown
Collaborator Author

Triage dispositions

In reply to comment_id: PRR_kwDOPatrT88AAAABHspOyg

Agreed, and the drift is not hypothetical — will be addressed by TASK-010 on this branch. Checking the EventTypeTest lists against EventType found the omission the comment predicts has already occurred: seventeen constants satisfy hasHttpMapping(), the CsvSource table carries sixteen rows, and WEBSOCKET_ORIGIN_REJECTED (AUTHORIZATION, 403, EventType.java:147) is asserted by nothing. WEBSOCKET_IDLE_TIMEOUT (EventType.java:153) is likewise absent from the success-event EnumSource. TASK-010 closes both gaps and adds the guard asserting the table's key set equals exactly the set of constants with an HTTP mapping, so a future mapped constant cannot be omitted silently. The three inline comments on this review are dispositioned on their own threads.

In reply to comment_id: IC_kwDOPatrT88AAAABMUcxmA

Acknowledged — no action required. The PR Reviewer Guide reports tests present, no security concerns and no major issues, so it carries no actionable finding to dispose. Noted and resolved.

cuioss-oliver and others added 5 commits July 29, 2026 21:33
…tional

The security_filter table row promised an application/problem+json envelope
unconditionally for every over-cap request, contradicting the "Shadowing at the
boundary" paragraph in the same section: when the framework floor equals the
largest declared cap, the framework's Content-Length pre-check answers with a
bare 413 before gateway request handling runs.

Qualify the envelope guarantee on the request reaching gateway enforcement and
cross-reference the shadowing paragraph. The 413 behaviour and the enforcement
order are unchanged.

Addresses PR #131 review finding f53145.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UnvLgrWg6jzDc1hNemnvb7
…ure drift

The @CsvSource error-contract table was a hand-maintained mirror of the
EventType constants carrying an HTTP mapping, and it had already drifted:
seventeen constants satisfy hasHttpMapping() while the table carried sixteen
rows, leaving WEBSOCKET_ORIGIN_REJECTED (AUTHORIZATION, 403) asserted by
nothing. WEBSOCKET_IDLE_TIMEOUT was likewise unclaimed by any list, so its
1001 Going Away close code was unasserted.

Close both gaps and make omission impossible:

* Add the WEBSOCKET_ORIGIN_REJECTED row, and give WEBSOCKET_IDLE_TIMEOUT its
  own assertion rather than folding it into the success list — it carries a
  null category and httpStatus 0 but a non-zero wsCloseCode.
* Move the four contract lists into shared constants driving @MethodSource, so
  each list has exactly one declaration. Expected statuses and categories stay
  stated as literals, never re-read from the enum under test.
* Add three drift guards: the error-contract table's key set must equal the
  hasHttpMapping() set, the WebSocket-close list must equal the non-zero
  wsCloseCode set, and the four lists must partition the enum with no constant
  unclaimed or double-claimed.

Addresses PR #131 review finding b21e8d.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UnvLgrWg6jzDc1hNemnvb7
Re-enabling uploadLarge took the executing set from eleven to twelve. Two of
the three count-prose sites in benchmark.yml were updated in that change; the
comment block above the "Summarise benchmark coverage" step was missed and
still read "3 of the 11 executing goals" / "all 11" while the expected list
below it holds twelve names.

Comment-only change; the step body, the expected list and the skip table are
untouched.

Addresses finding b4790e, found during ground-truth verification of PR #131
review finding f0867e.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UnvLgrWg6jzDc1hNemnvb7
Finding 8525f1. The negative case asserted only the RFC 9457 envelope, on the
reasoning that the status is incidental once the gateway hands over a rejection.
It is not incidental: it is the mapping this PR changes. A regression that moved
the event back to a 400-mapped constant would have left the envelope assertions
green.

Assert both halves, and say why neither replaces the other: the status locks the
CONTENT_TOO_LARGE -> 413 mapping, while the envelope is what discriminates the
gateway's rejection from the framework's own Content-Length pre-check, which
answers with a bare 413 carrying no envelope.

Verified against the containerised stack: LargeBodyIT 2/2 green.

Co-Authored-By: Claude <noreply@anthropic.com>
…anifest

Finding f0867e. The benchmark coverage step maintained a hard-coded expected goal
list plus a hand-maintained skip table, both mirroring the resolved -Pbenchmark
execution set in benchmarks/pom.xml with nothing enforcing agreement. The drift
was not hypothetical: this PR moved the executing set from eleven to twelve and
left prose in the same file saying eleven.

Add benchmarks/scripts/benchmark-manifest.py, bound to initialize so the manifest
exists before any goal runs and a suite truncated by a later failure still states
what was expected. Maven hands over only the RESOLVED value of each skip
property, so a -Dskip.benchmark.upload.large=true override is reflected; the
generator exits non-zero when an execution's skip property was not handed over,
so a new suppressible goal cannot silently report as executing.

The workflow now derives both the gated set and the not-expected table from the
manifest, and treats an absent manifest as a hard failure rather than an empty
expected set that would vacuously report full coverage. The unwired
sessionMediated script carries its own @unwiredReason tag, so that reason lives in
one place instead of being duplicated into the workflow.

Scope note: this exceeds the plan's D6 (re-enable uploadLarge). It was
implemented at explicit operator direction after triage recommended deferring it
to a follow-up.

Verified: manifest generates via Maven initialize with 12 enabled / 1 unwired and
uploadLarge enabled; both jq expressions checked against the real manifest; the
missing-skip-property guard exits 1. The workflow step itself only runs in a
post-merge Performance Benchmark run and is unproven until then.

Co-Authored-By: Claude <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
.github/workflows/benchmark.yml (1)

139-217: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Use one authoritative manifest coverage summary implementation.

.github/workflows/benchmark.yml reimplements the CI coverage step in bash/jq, while benchmarks/scripts/benchmark-manifest.py provides a summarise subcommand that is documented as the CI consumer but is not invoked. This creates a second copy of the same contract: the bash output uses ### Not expected to run, adds a raw state column, and treats <skip>true</skip> entries as “not wired”; the Python implementation uses ### Deliberately not run with Skipped by/Reason columns and labels inline skips correctly. Either call benchmark-manifest.py summarise from the workflow, or remove the dead summarise() CLI/subcommand if bash is intended to remain authoritative.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 11d08dc5-b818-47be-89cd-81ad1db0100a

📥 Commits

Reviewing files that changed from the base of the PR and between f9112fa and de169eb.

📒 Files selected for processing (8)
  • .github/workflows/benchmark.yml
  • api-sheriff/src/test/java/de/cuioss/sheriff/gateway/events/EventTypeTest.java
  • benchmarks/README.adoc
  • benchmarks/pom.xml
  • benchmarks/scripts/benchmark-manifest.py
  • benchmarks/src/main/resources/k6-scripts/session_mediated.js
  • doc/configuration.adoc
  • integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/LargeBodyIT.java
🚧 Files skipped from review as they are similar to previous changes (1)
  • benchmarks/README.adoc

cuioss-oliver and others added 2 commits July 30, 2026 07:31
… in bash

Review finding on the previous commit, and it is correct. benchmark-manifest.py
already ships a summarise subcommand — its module docstring names it as the CI
consumer — but the workflow reimplemented the same rendering in bash/jq. That
recreated, one level up, the duplicated-contract defect the manifest was
introduced to remove: two implementations of one contract, already diverged on the
table heading, and the jq fallback mislabelled an inline <skip>true</skip> entry
as "not wired" where the Python names the property.

Delete the bash/jq block and call summarise, so the manifest has exactly one
consumer.

Verified all three exit semantics: complete coverage exits 0; a missing summary
document with a successful benchmark step exits 1; an absent manifest exits 1 when
the benchmark step succeeded and 0 when it failed, deferring to Maven's own error
rather than masking it.

Co-Authored-By: Claude <noreply@anthropic.com>
Review finding on EventTypeTest:114. The success, configuration and
WebSocket-close sets were three hand-maintained mirrors of EventType, and the
drift guards added earlier only detect an omission after that second registry has
already drifted. A derived set cannot drift at all.

EventType already exposes the classifying accessors, so no new production API was
needed: hasHttpMapping(), wsCloseCode() and category() partition the enum into the
four buckets the suite asserts, and membership is now selected by predicate over
allOf(EventType.class). A constant added to the enum lands in its bucket
automatically.

What stays literal is the part that carries independent information: ERROR_CONTRACT
still states each mapped event's expected status and category as written-out
values, so the mapping assertions cannot pass by re-reading the enum under test.
Derive the membership, state the mapping.

Two consequential follow-ons rather than a mechanical swap:

  * the WebSocket-close case was a single hard-coded constant, so a second
    close-code constant would have gone unasserted. It is now parameterized over
    the derived set, with the literal 1001 kept as its own assertion since the
    parameterized test derives membership from wsCloseCode() and therefore cannot
    also be what pins the value.
  * the ws-close coverage guard became tautological once its set was derived, and
    is removed. The partition guard is retained and re-documented: with membership
    derived it no longer guards a forgotten list entry, it guards the PREDICATES —
    a constant claimed by none or by two shapes means they have stopped
    partitioning the enum.

Verified: derived counts match the previously hand-listed sets exactly (7 success,
2 configuration, 1 WebSocket-close, 18 error-contract), the partition guard passes,
and no bucket derives empty.

Co-Authored-By: Claude <noreply@anthropic.com>
@cuioss-oliver

Copy link
Copy Markdown
Collaborator Author

Triage dispositions

In reply to comment_id: PRR_kwDOPatrT88AAAABHweL1g — "Use one authoritative manifest coverage summary implementation"

Correct, and this was my error. Fixed in eeec70b.

benchmark-manifest.py already shipped a summarise subcommand — its module docstring names it as the CI consumer — and I wired the workflow in bash/jq without reading the generator I was wiring up. That recreated, one level up, the exact duplicated-contract defect the manifest was introduced to remove: two implementations of one contract, already diverged.

Your diagnosis of the divergence was precise on both counts. The bash version used ### Not expected to run with a raw state column against the Python's ### Deliberately not run with Skipped by/Reason; and my .skip_property // "not wired" fallback mislabelled an inline <skip>true</skip> entry as "not wired", where _skipped_by() correctly renders it as an inline `<skip>` . The Python is also better in a way the comment doesn't mention: _not_run_reason() emits a remediation hint naming the missing .reason property, which the bash silently rendered as -.

The bash/jq block is deleted and the workflow now calls summarise. One consumer, no dead CLI surface.

Verified all three exit semantics rather than assuming them, since the gate's value depends on them: complete coverage exits 0; a missing summary document with a successful benchmark step exits 1; an absent manifest exits 1 when the benchmark step succeeded and 0 when it failed, deferring to Maven's own error rather than masking it with a derived one.

In reply to comment_id: PRR_kwDOPatrT88AAAABHsl0QQ (sourcery-ai)

No action required — a weekly rate-limit notice, not a review finding. Sourcery is configured as an optional bot on this repo, so its hard_quota refusal does not gate the merge; CodeRabbit and PR-Agent both participated. Recorded and resolved so it does not read as an unaddressed comment.

The plan settled a real architectural question rather than fixing a local bug:
whether a declared max_body_bytes is the effective ceiling, and which component
owns an over-cap rejection. The spec asked for the choice to be stated with its
reasoning, and the alternatives were weighed and discarded on their merits, so the
decision belongs in an ADR rather than only in a commit message.

Records the derivation plus fail-closed boot validation, the 413 rendering on both
the proxy and gRPC paths, and — as consequences of the resulting architecture
rather than incident notes — the shadowing behaviour at floor == cap, the differing
enforcement path for chunked requests, and the invariant that raising the framework
floor must never become the only ceiling for a route.

Co-Authored-By: Claude <noreply@anthropic.com>
@cuioss-oliver
cuioss-oliver added this pull request to the merge queue Jul 30, 2026
Merged via the queue into main with commit 29f1dac Jul 30, 2026
24 checks passed
@cuioss-oliver
cuioss-oliver deleted the feature/plan-33-body-cap-derivation branch July 30, 2026 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant