Feat: Config file for Context Guru that terminates TLS - #720
Conversation
…orts Signed-off-by: Ed Snible <snible@us.ibm.com>
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe proxy now binds explicit TCP listeners and logs their actual addresses. The context-guru TLS-bridge demo is documented as a standalone HTTPS variant, uses forward-only listener wiring and port-based interception, and scopes ChangesProxy and TLS bridge integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@authbridge/demos/context-guru/context-guru-tls-bridge.yaml`:
- Around line 53-56: Move the on_error setting for the context-guru plugin from
the plugin entry level into its config mapping, keeping the value unchanged.
Ensure all plugin-specific settings in this pipeline entry follow the
pipeline.*.plugins[].config structure expected by the runtime loader.
- Line 29: Update the forward_proxy_addr setting in the context-guru TLS bridge
configuration to bind the unauthenticated forward proxy explicitly to loopback,
using the documented localhost address while preserving port 8081.
🪄 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 Plus
Run ID: e478868b-6fa8-4caf-960c-bcca7947cf0c
📒 Files selected for processing (2)
authbridge/cmd/authbridge-proxy/main.goauthbridge/demos/context-guru/context-guru-tls-bridge.yaml
Code reviewFound 2 issues:
cortex/authbridge/demos/context-guru/context-guru-tls-bridge.yaml Lines 43 to 47 in 38f3872
cortex/authbridge/demos/context-guru/context-guru-tls-bridge.yaml Lines 16 to 19 in 38f3872 Also worth a look (below the reporting bar, not blocking): 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
huang195
left a comment
huang195
left a comment
There was a problem hiding this comment.
Summary
Small, well-commented PR: a standalone HTTPS variant of the context-guru demo config plus an accurate-bound-address logging fix in authbridge-proxy. The Go change is a real improvement (bind failures are now synchronous instead of an async log.Fatalf inside a goroutine), and I verified the whole tls_bridge block against the schema. One blocking issue: the passthrough_hosts wildcards are inert, so the config asserts a containment property it doesn't have.
Author: esnible (MEMBER — maintainer)
Areas reviewed: Go (cmd/authbridge-proxy), YAML demo config, cross-checked against authlib/config, authlib/tlsbridge, listener/forwardproxy, demo README + run.sh
Agent/IDE config (.claude/.vscode): none
Commits: 1 commit, all signed-off: yes
CI status: passing (20/20)
Verified as correct — no action needed
tls_bridgefields (mode,ca_dir,generate_ca,upstream_ca_bundle,ports) all matchconfig.TLSBridgeConfig.on_error: enforce | observe | offis the real valid set (config.go:294).-tags include_plugin_contextguruis right — contextguru is genuinely opt-in, unlike theexclude_plugin_*majority.${CG_MODEL_*}expansion works (os.Expandatconfig.go:507).netwas already imported;srv.Shutdownstill closes the listener passed toServe.- Nothing in the repo greps the
"HTTP server listening"string, so the rename to"Reverse server listening"is safe.
| # upstream_ca_bundle: /path/to/roots.pem # extra roots for re-origination to a | ||
| # private-CA LLM endpoint; omit for system roots only. | ||
| passthrough_hosts: # hosts to tunnel WITHOUT terminating (egress gate still | ||
| - "keycloak.*" # runs, they just aren't decrypted). Keep control-plane |
There was a problem hiding this comment.
must-fix — these wildcards never match anything, so the stated intent ("Keep control-plane and IdP traffic out of the MITM path") silently does not hold.
passthrough_hosts is an exact string set, not a glob or regex:
tlsbridge.NewDecisionbuildsskip := map[string]bool{}and fills it withd.skip[h] = true(authbridge/authlib/tlsbridge/decision.go:26-33).Classifytests it with a plain map lookup:if d.skip[host] { return Passthrough, "skip" }(decision.go:53).- The key handed in is a bare hostname —
key := hostOnly(r.Host)(authbridge/authlib/listener/forwardproxy/server.go:1004, same intransparent.go:141).
So a connection to keycloak.keycloak.svc.cluster.local is looked up as that literal string, misses both entries, and gets terminated. A host literally named keycloak.* does not exist.
This does not break this demo (it's the no-auth variant with inbound.plugins commented out, so there's no Keycloak traffic either way) — but a demo config is teaching material and this is the kind of line that gets copy-pasted into a config where it does matter.
Suggested fix — use exact hostnames, which is what every other example in the tree does (authbridge/authlib/config/config_test.go:816 uses pinned.example.com; the design doc at docs/superpowers/specs/2026-06-12-authbridge-tlsbridge-design.md:268 shows an empty list):
passthrough_hosts: # hosts to tunnel WITHOUT terminating (egress gate still
- "keycloak.keycloak.svc.cluster.local" # runs, they just aren't decrypted). Exact
- "keycloak.keycloak.svc" # match only — no globs. Keep control-plane
# and IdP traffic out of the MITM path.If suffix/wildcard matching is actually wanted, that's a decision.go change (build a matcher instead of a map) — worth a separate issue, and the config comment should not imply it exists until then.
| mode: proxy-sidecar | ||
| listener: | ||
| # Note that commenting out the reverse proxy doesn't disable it, so give it a random port. | ||
| reverse_proxy_addr: ":0" |
There was a problem hiding this comment.
suggestion — the :0 workaround isn't necessary; there's a first-class way to run egress-only.
listener.roles selects which proxies start, and config.go:379 documents this exact scenario verbatim:
// roles: [forward] # egress-only (e.g. a laptop TLS-bridge demo)
// roles: [reverse] # inbound-only JWT validationActiveRoles() defaults to both when the list is empty (config.go:476-486), which is why commenting the block out doesn't disable anything — but naming the role does.
listener:
roles: [forward] # egress-only: no reverse proxy at all
forward_proxy_addr: ":8081"That drops the :0, the placeholder reverse_proxy_backend, and the stray random-port bind, and it teaches the supported knob rather than the workaround. roles: currently appears in zero configs in the repo, so this demo would be a good first place to show it off.
| Handler: handler, | ||
| ReadHeaderTimeout: 10 * time.Second, | ||
| } | ||
| listener, err := net.Listen("tcp", addr) |
There was a problem hiding this comment.
suggestion — cmd/authbridge-cpex/main.go:315-346 carries a byte-identical copy of both functions, still on srv.ListenAndServe() and still logging the requested addr. Same :0 blind spot, same async-fatal-on-bind-failure.
Worth applying the same change there so the two binaries don't drift — or a line in the commit message noting cpex is deliberately out of scope.
(The change itself reads well: hoisting net.Listen out of the goroutine also turns a bind failure into a synchronous fatal, which is strictly better than the old behavior of logging "listening" and then dying from inside the goroutine.)
| # Note that commenting out the reverse proxy doesn't disable it, so give it a random port. | ||
| reverse_proxy_addr: ":0" | ||
| forward_proxy_addr: ":8081" | ||
| reverse_proxy_backend: "http://localhost:8001" # the agent (moved off :0) |
There was a problem hiding this comment.
nit — # the agent (moved off :0) parses as "the agent was moved off port 0", which isn't a thing. I think it means "the agent's real port, as distinct from the :0 two lines up". Reword or just drop the parenthetical — and it becomes moot entirely if you take the roles: [forward] suggestion above.
| @@ -0,0 +1,83 @@ | |||
| # AuthBridge config for the context-guru demo — HTTPS variant. | |||
There was a problem hiding this comment.
nit — nothing references this file, so it's hard to find.
The demo README's "Configuration reference" section documents k8s/authbridge-config.yaml and k8s/agent.yaml, and run.sh only ever templates k8s/authbridge-config.yaml (line 74's sed). This new file sits at the demo root and is mentioned nowhere.
A one-liner in the README — something like "context-guru-tls-bridge.yaml — standalone/laptop HTTPS variant (tls_bridge + HTTPS_PROXY), not used by run.sh" — would make the split obvious. The root-vs-k8s/ placement is a reasonable signal on its own, it just needs the pointer.
| go func() { | ||
| slog.Info("HTTP server listening", "name", name, "addr", addr) | ||
| if err := srv.ListenAndServe(); err != nil && err != http.ErrServerClosed { | ||
| actualAddr := listener.Addr().String() |
There was a problem hiding this comment.
nit — actualAddr is used exactly once on the next line. Inlining listener.Addr().String() into the slog.Info call matches the sibling startReverseProxyServer edit in this same diff, which does it inline.
Signed-off-by: Ed Snible <snible@us.ibm.com>
Signed-off-by: Ed Snible <snible@us.ibm.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@authbridge/demos/context-guru/context-guru-tls-bridge.yaml`:
- Around line 48-50: Update the context-guru configuration documentation in
README.md to use the runtime path config.on_error (or
context-guru.config.on_error) instead of context-guru.on_error, and adjust any
related editing commands to target the same nested path.
🪄 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 Plus
Run ID: a2f0059c-8ac5-4e1f-9f78-630aa87e7862
📒 Files selected for processing (3)
authbridge/cmd/authbridge-proxy/main.goauthbridge/demos/context-guru/README.mdauthbridge/demos/context-guru/context-guru-tls-bridge.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- authbridge/cmd/authbridge-proxy/main.go
| - name: context-guru | ||
| config: | ||
| on_error: enforce # enforce | observe | off (see header) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Update the documented on_error path.
This YAML now stores the setting under plugins[].config, but README.md still documents context-guru.on_error. Following that table can place the setting where the runtime ignores it. Rename the table entry to config.on_error (or context-guru.config.on_error) and align any editing commands with that path.
Suggested documentation fix
-| `context-guru.on_error` | `enforce` (apply) / `observe` (shadow-measure) / `off` (kill-switch) |
+| `config.on_error` | `enforce` (apply) / `observe` (shadow-measure) / `off` (kill-switch) |🤖 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 `@authbridge/demos/context-guru/context-guru-tls-bridge.yaml` around lines 48 -
50, Update the context-guru configuration documentation in README.md to use the
runtime path config.on_error (or context-guru.config.on_error) instead of
context-guru.on_error, and adjust any related editing commands to target the
same nested path.
Source: Coding guidelines
huang195
left a comment
huang195
left a comment
There was a problem hiding this comment.
Summary (round 2 — head 310de39)
Thanks for the quick turnaround — five of the six round-1 findings are resolved, and the cpex copy now matches the proxy one line for line (including the net import). One problem: the Address review comments commit also moved on_error into the plugin's config: block, which silently disables two of the demo's three modes. That wasn't part of the round-1 feedback, and nothing in CI or at startup will tell you it's wrong.
Author: esnible (MEMBER — maintainer)
Areas reviewed: Go (cmd/authbridge-proxy, cmd/authbridge-cpex), YAML demo config, Docs (demo README); cross-checked against authlib/config, authlib/pipeline, authlib/plugins/contextguru, authlib/tlsbridge
Agent/IDE config (.claude/.vscode): none
Commits: 3 commits, all signed-off: yes
CI status: passing (all checks)
Round-1 findings — status
| Round-1 finding | Status |
|---|---|
must-fix: passthrough_hosts wildcards were inert |
✅ resolved — block removed entirely |
suggestion: use roles: [forward] instead of the :0 hack |
✅ adopted |
suggestion: same fix needed in authbridge-cpex/main.go |
✅ done in 310de39 |
nit: (moved off :0) comment |
|
| nit: README pointer to the new file | ✅ added |
nit: inline actualAddr |
✅ done |
The remaining blocker is the new on_error placement, not anything from round 1.
| - name: inference-parser | ||
| - name: context-guru | ||
| config: | ||
| on_error: enforce # enforce | observe | off (see header) |
There was a problem hiding this comment.
must-fix (regression in 04b76e0) — on_error moved from the plugin entry into the plugin's config: block. It needs to go back out one level.
on_error is framework-owned and parsed at the entry level, not inside config:
type PluginEntry struct {
Name string `yaml:"name" json:"name"`
ID string `yaml:"id,omitempty" json:"id,omitempty"`
OnError pipeline.ErrorPolicy `yaml:"on_error,omitempty" json:"on_error,omitempty"`
Config json.RawMessage `yaml:"-" json:"config,omitempty"`
}and the struct's own doc comment is explicit about it (authlib/config/config.go:250-253):
OnError is the framework-owned wrapper policy (see ErrorPolicy). Plugin authors do not consume it — it lives outside the plugin's own config block so all plugins get the same rollout story without each one re-implementing shadow mode.
Why this fails silently rather than loudly:
PluginEntry.UnmarshalYAMLonly recogniseson_erroras a key of the entry mapping (config.go:287-296). Nested one level down it's just part of the opaqueconfigsubtree.- context-guru has no such field —
git grep -E 'on_error|OnError' authbridge/authlib/plugins/contextguru/returns zero hits. ContextGuru.Configuredecodes with plainjson.Unmarshal, notDisallowUnknownFields(plugins/contextguru/plugin.go:166-171), so the stray key is dropped without error.- Nothing warns —
config/warn.goonly hasWarnEmptyPipelines. - The framework's
OnErroris therefore the empty string, and an empty policyResolved()s toErrorPolicyEnforce(pipeline/policy.go:58, andpipeline/pipeline.go:23-24: "An empty ErrorPolicy resolves to ErrorPolicyEnforce").
So the net effect is the nastiest kind: enforce keeps working by accident (it's the default), while observe and off become silent no-ops. off never reaches the if policy == ErrorPolicyOff { continue } skip (pipeline.go:106,149,199) and observe never reaches the mutation-muting branches (context.go:419,442) — so you can't get the baseline and you can't shadow-measure.
That is the whole point of this demo. The README's three-row table (off → truncated + wrong answer, observe → shadow-measures the gain, enforce → correct answer) and this file's own header — "THREE MODES via the context-guru entry's on_error" — both describe behaviour that no longer happens. The sibling k8s/authbridge-config.yaml still has it in the right place, so the two configs have silently diverged.
- name: context-guru
on_error: enforce # enforce | observe | off (see header)
config:
# Only inference paths are compacted; everything else passes through.
paths: ["/v1/chat/completions", "/v1/completions", "/v1/messages"]Worth a follow-up issue independently: a misplaced on_error is currently undetectable. Either have the framework warn when a plugin's raw config contains a reserved framework key, or give context-guru a DisallowUnknownFields decode so the typo fails at boot.
| mode: proxy-sidecar | ||
| listener: | ||
| roles: [forward] # egress-only: no reverse proxy at all | ||
| forward_proxy_addr: "localhost:8081" |
There was a problem hiding this comment.
suggestion — keep the loopback intent, but use the literal IP: 127.0.0.1:8081.
Narrowing from :8081 to loopback-only is a genuine improvement, and worth calling out — :8081 binds every interface, so a laptop demo would be running an open forward proxy reachable from the LAN.
The hostname form is the risk. net.Listen("tcp", "localhost:8081") resolves the name and binds a single address, so on a dual-stack host where ::1 sorts ahead of 127.0.0.1 you get an IPv6-only listener. This file's own prerequisite two dozen lines up says:
# 2. Agent routes HTTPS through the forward proxy:
# HTTPS_PROXY=http://127.0.0.1:8081
which would then fail to connect, with a confusing "connection refused" against a proxy that the logs just said was listening. Nothing catches this — the field goes straight to net.Listen, and config/presets.go:14,25 only ever sets defaults for it, there's no format validation.
forward_proxy_addr: "127.0.0.1:8081" is deterministic and matches the documented HTTPS_PROXY exactly.
| listener: | ||
| roles: [forward] # egress-only: no reverse proxy at all | ||
| forward_proxy_addr: "localhost:8081" | ||
| # reverse_proxy_backend: "http://localhost:8001" # the agent (moved off :0) |
There was a problem hiding this comment.
nit — the commented-out line still carries # the agent (moved off :0), but there's no longer a :0 anywhere in this file for it to refer to, so the parenthetical now points at nothing.
Suggest deleting the line outright rather than commenting it: with roles: [forward] there is no reverse proxy, so there's no backend for it to have. A reader who uncomments it will get a field that's silently ignored.
Signed-off-by: Ed Snible <snible@us.ibm.com>
huang195
left a comment
huang195
left a comment
There was a problem hiding this comment.
Summary (round 3 — head de1ee68)
LGTM. de1ee68 restores on_error to the plugin-entry level at the correct indentation — sibling of name/config, byte-identical placement to k8s/authbridge-config.yaml:26, so the framework's PluginEntry.OnError is populated again and all three modes (enforce/observe/off) work as the header comment and the README table describe. The two configs no longer diverge.
The delta since round 2 is exactly that one-line move; both Go files are unchanged.
Author: esnible (MEMBER — maintainer)
Areas reviewed: Go (cmd/authbridge-proxy, cmd/authbridge-cpex), YAML demo config, Docs (demo README); cross-checked against authlib/config, authlib/pipeline, authlib/plugins/contextguru, authlib/tlsbridge
Agent/IDE config (.claude/.vscode): none
Commits: 4 commits, all signed-off: yes
CI status: passing (18 pass, Spellcheck skipped, 0 failures)
Full findings ledger across the three rounds
| Finding | Status |
|---|---|
must-fix: passthrough_hosts wildcards were inert (exact-match only) |
✅ resolved — block removed |
must-fix: on_error inside config: silently disabled observe/off |
✅ resolved in de1ee68 |
suggestion: roles: [forward] instead of the :0 reverse-proxy hack |
✅ adopted |
suggestion: same listener fix needed in authbridge-cpex/main.go |
✅ done in 310de39 |
suggestion: localhost:8081 → 127.0.0.1:8081 |
⬜ open (advisory — see inline) |
nit: stale (moved off :0) comment |
⬜ open (advisory — see inline) |
| nit: README pointer to the new file | ✅ added |
nit: inline actualAddr |
✅ done |
The two open items are advisory and don't block — approving on that basis. Both are one-liners if you want to fold them in before merge.
The listener change itself is a genuine improvement worth noting for anyone reading this later: hoisting net.Listen out of the goroutine turns a bind failure into a synchronous fatal instead of logging "listening" and then dying asynchronously, and it makes :0 (and any other ephemeral bind) report the real port.
Separately, worth a follow-up issue independent of this PR: a misplaced on_error is currently undetectable — UnmarshalYAML only recognises it at the entry level, context-guru's Configure decodes with plain json.Unmarshal rather than DisallowUnknownFields (plugins/contextguru/plugin.go:166-171), and config/warn.go only warns on empty pipelines. Either a framework warning when a plugin's raw config contains a reserved framework key, or a strict decode in the plugin, would have caught the round-2 slip at boot instead of silently defaulting to enforce.
| mode: proxy-sidecar | ||
| listener: | ||
| roles: [forward] # egress-only: no reverse proxy at all | ||
| forward_proxy_addr: "localhost:8081" |
There was a problem hiding this comment.
suggestion (carried from round 2, non-blocking) — consider 127.0.0.1:8081 rather than localhost:8081.
Narrowing from :8081 to loopback is the right instinct and worth keeping: :8081 binds every interface, so a laptop demo would expose an open forward proxy to the LAN.
The hostname form is the part I'd change. net.Listen("tcp", "localhost:8081") resolves the name and binds a single address, so on a dual-stack host where ::1 sorts ahead of 127.0.0.1 you end up with an IPv6-only listener — while this file's own prerequisite says:
# 2. Agent routes HTTPS through the forward proxy:
# HTTPS_PROXY=http://127.0.0.1:8081
That combination fails with a "connection refused" against a proxy the logs just announced as listening, which is an annoying thing to debug in a demo. Nothing guards it either — the value goes straight to net.Listen, and config/presets.go:14,25 only ever sets defaults for the field; there's no format validation.
The literal IP is deterministic and matches the documented HTTPS_PROXY exactly. Not blocking.
| listener: | ||
| roles: [forward] # egress-only: no reverse proxy at all | ||
| forward_proxy_addr: "localhost:8081" | ||
| # reverse_proxy_backend: "http://localhost:8001" # the agent (moved off :0) |
There was a problem hiding this comment.
nit (carried from round 2, non-blocking) — this commented-out line still carries # the agent (moved off :0), but there's no longer a :0 anywhere in the file for the parenthetical to refer to.
Suggest deleting the line outright instead of leaving it commented: with roles: [forward] there is no reverse proxy, so there's no backend for it to point at. Anyone who uncomments it gets a field that's silently ignored.
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
3 participants
Summary
This PR contains an example config file in support of a Context Guru demo being written for the documentation.
It also adds logging of random ports.
Summary by CodeRabbit
New Features
Improvements
Documentation