oscerd commented on PR #26670:
URL: https://github.com/apache/camel/pull/26670#issuecomment-5828711785
@gnodet — not applying either of these two, and I want to show the evidence
rather than just disagree.
**1. Health-check ID assertion is not hostname-tied.** The line reads:
```java
assertThat(checks.get(0).getId()).startsWith("security-policy:opa-").contains("opa-rest");
```
It asserts the ID *contract* (`security-policy:opa-` prefix, which
`OpaSecurityPolicyHealthCheck` builds from the sanitized serverUrl/policyPath)
**and** the hostname. The comment directly above says so:
> Assert the full ID contract, not just the hostname, so a refactor of the
ID-building logic cannot pass here silently
So the thing the finding asks for is already there. If the ask is to drop
`.contains("opa-rest")` entirely, that would weaken it: the test's whole point
is distinguishing the rest policy's check from the wasm policy's absent one,
and the hostname is what identifies *which* policy registered it.
**2. The corrupt-bundle assertion was reviewed and deliberately kept.**
@davsclaus looked at exactly this on 2026-09-23 and concluded:
> `RuntimeException` over `Exception` is only marginally tighter, and an
unrelated NPE would still pass. But the comment now explains *why* there's
nothing better to assert — the module rejection comes out of `OpaPolicy`'s
warmup borrow and `buildEvaluator` rethrows it as-is, so any message assertion
would be pinned to a Chicory/OPA version. Given that, I'd rather have the
explanation than a brittle `hasMessageContaining`, so this is fine by me.
The companion test below it *does* assert `RuntimeCamelException` plus the
message, because that path goes through `beforeWrap`'s wrapping — which is the
case worth pinning. Tightening this one to match would assert a wrapping that
does not happen on this path.
Two AI reviewers now disagree on the same line, and the earlier one had the
reasoning in front of it. I am going with that, but if you would rather
overrule it, say so and I will change it.
Nothing else outstanding from my side here: the `opaClient` parity fix is in
`0b922966`, CI is green, and the `authz.wasm` merge-order note is
cross-referenced on #26677.
_Claude Code on behalf of @oscerd_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
