Repository navigation
fix(gcp): expose read-API /healthz and accept deny-502 in native proof - #207
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 28 minutes. View limit details
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. |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
| - path: /healthz | ||
| pathType: Exact | ||
| backend: | ||
| service: { name: {{ $nativeReadEdge }}, port: { number: 80 } } |
There was a problem hiding this comment.
🟡 Unbumped chart blocks health-route release
Adding /healthz changes the chart while its version remains 0.6.22. The version-bump check fails, blocking the route's release.
Learn more
The chart's version is still 0.6.22 in Chart.yaml. The version-bump check compares the changed chart to the base and fails when the version is unchanged. The release process also uses chart versions to identify publishable changes.
Example: A PR changes the native History ingress but leaves version 0.6.22. The CI version check fails before the new route can ship.
Recommended fix: Increase the agent-relay chart version in charts/agent-relay/Chart.yaml, then update any associated chart-version references required by the repository's release checks.
Was this helpful? React with 👍 or 👎 to provide feedback.
| - path: /healthz | ||
| pathType: Exact | ||
| backend: | ||
| service: { name: {{ $nativeReadEdge }}, port: { number: 80 } } |
There was a problem hiding this comment.
🟡 Health route lacks regression coverage
Adding /healthz changes native History routing without a test asserting its backend. The native ingress test checks other routes, so this behavior remains unprotected.
Learn more
The changed route is part of the generated GKE Ingress in native History mode. The existing native ingress test inspects rendered paths but does not assert the new health path. CONTRIBUTING.md explicitly requires adding or updating tests for changed behavior; the proof's newly accepted 502 response also has no added regression coverage.
Example: If a later edit points /healthz back to the deny service, the current native-profile route assertions still pass even though the public verifier receives 502.
Recommended fix: Extend the native GKE ingress render test to assert an Exact /healthz route to the native read edge on port 80, with the fallback and internal paths still denied. Add a focused proof test for the new 502 acceptance where the proof harness supports it.
Was this helpful? React with 👍 or 👎 to provide feedback.
[Medium impact] The PR appears safe to merge; no new actionable issues were found.
|
Two live-proof findings on the native GKE edge: - verify-public.sh requires a 2xx on history.<base>/healthz, but the native umbrella routes every unmatched path to the selector-less deny Service (Google edge answers 502). Route Exact /healthz to the read edge -- the same probe the GCE health check already uses -- so the public health contract holds without widening any /v1 surface. - e2e public.history_internal_denied expected 404/403 on /internal/v1/session-read/sessions, but the umbrella's catch-all is the selector-less deny backend, which yields 502 -- identical to the cast deny cases that already accept 502 as edge_denied. Accept 502 here too; the security property (internal path never reaches the read API) is unchanged. Review: adversarial -- exposing /healthz adds no surface beyond the probe GCE already hits; accepting 502 does not weaken the isolation assertion. Contextual -- matches the deny design and sibling checks. Validation: helm template renders the new Exact rule; live edge confirmed 502-before/expected-404 mismatch. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
9e04b9f to
1d88864
Compare
Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
… publish) Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Summary
verify-public.shrequires 2xx onhistory.<base>/healthz. Adds an Exact/healthzroute to the native read edge — the same probe the GCE health check already uses.public.history_internal_deniedexpected 404/403 but the deny backend answers 502; sibling deny cases already accept 502 asedge_denied. Made this case consistent. The isolation property (internal paths never reach the read API) is unchanged.Validation
helm templaterenders the new Exact/healthzrule./healthz→ 502 before the change,public.history_internal_deniedwas the only verifier gap remaining (verifier was 15/17, the other being this case and the history health URL).Generated with Devin
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.Summary by cubic
Fixes two live-proof failures on the native GKE edge so
verify-public.shpasses./healthzroute to the native read edge; previously the umbrella routed it to the selector-less deny Service and Google answered 502.public.history_internal_deniedsince the deny backend answers 502; internal paths still never reach the read API, so the isolation property is unchanged.agent-relayto0.6.52, registers it as the current release row, and marks0.6.51as planned.Written for commit 40c3a1f. Summary will update on new commits.