Feat: context-guru context-engineering plugin + demo#660
Conversation
|
Warning Review limit reached
Next review available in: 43 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 ignored due to path filters (5)
📒 Files selected for processing (16)
✨ 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 |
5f569e8 to
0ff26fa
Compare
0ff26fa to
c1c64f1
Compare
Add context-guru as an in-process AuthBridge outbound plugin that compacts an
agent's LLM request context (tool outputs) before it is forwarded upstream, plus
a runnable end-to-end demo on Kagenti.
- authlib/plugins/contextguru: OnRequest compaction via apply.BodyWithModel +
SetBody; slog logging; WritesBody + RequiresAny:[inference-parser]; tests.
- cmd/authbridge-{proxy,envoy}: build-tag-gated plugin registration.
- demos/context-guru: multi-transaction audit demo (off/observe/enforce),
README with diagrams + config reference, run.sh.
- demos/finance-sparc/finance-mcp: two large-output tools for realistic context.
Depends on the published github.com/kagenti/context-guru module. Toolchain
bumped to go 1.26.4 (required by context-guru via bifrost/core); Dockerfile
builders moved to golang:1.26-alpine (digest-pinned).
Relates to rossoctl#659, rossoctl#641.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Osher-Elhadad <Osher.Elhadad@ibm.com>
c1c64f1 to
aeeac62
Compare
mrsabath
left a comment
mrsabath
left a comment
There was a problem hiding this comment.
Strong PR — clean, thoroughly documented plugin with genuinely assertive tests (defaults, bad-engine rejection, provider mapping, real compaction, pass-throughs, and RequiresAny ordering). I read the plugin core directly and verified the key claims: api_key is sourced from a Secret via ${CG_MODEL_KEY} env expansion (never a ConfigMap; run.sh doesn't log it), context-guru is pinned to a real main pseudo-version (b624f2c3, no local replace), Go 1.26.4 is consistent across go.work + all go.mods, and the finance-mcp additions are purely additive with bounded output.
All findings below are non-blocking suggestions — nothing here blocks merge. I'm holding the formal approval for now (posting as comments); happy to convert to an approve once you've had a look.
Note (not inlinable — outside the diff): in authbridge/cmd/authbridge-envoy/Dockerfile:30, the docker.io/envoyproxy/envoy:v1.37.1 stage is pinned by tag only, while the file's other three FROM stages are digest-pinned. It predates this PR, but since you're already touching this Dockerfile, pinning it by digest would bring it to parity.
Other nits: run.sh's inline generated Dockerfile uses unpinned alpine:3.20 (the committed Dockerfiles are digest-pinned); the secret value transits kubectl create argv (briefly visible in the process table on a shared host); and Configure silently degrades to deterministic if only one of model.base_url / model.model is set — a typo'd-empty model name disables the cheap model with no error.
Areas reviewed: Go (plugin core + tests), Helm/K8s (demo manifests), Shell (run.sh), Dockerfiles, dependency bump (cross-repo verified against the context-guru repo). Commits: 1, signed-off (DCO green). CI: all checks passing.
|
|
||
| // sessionID keys per-session engine state to the conversation: the A2A contextId | ||
| // when present, else the caller subject for multi-tenant isolation. | ||
| func sessionID(pctx *pipeline.Context) string { |
There was a problem hiding this comment.
suggestion — sessionID falls back to "" when neither Session.ID nor Identity is present. Every such request then shares a single engine-store key (""), so per-session compaction state can bleed across unrelated callers. The comment describes the fallback but not the collision. Consider skipping compaction (or deriving a per-request random key) when there's no session identity, to preserve the multi-tenant isolation this is aiming for.
There was a problem hiding this comment.
Good catch — fixed in #672. OnRequest now skips compaction when there's no session identity (returns Continue, logs at DEBUG) rather than falling back to the shared "" store key, so per-session state can't bleed across callers. Also deduped the repeated sessionID(pctx) calls into one sid. Covered by TestOnRequest_NoSession_Skips.
| func (p *ContextGuru) OnRequest(ctx context.Context, pctx *pipeline.Context) pipeline.Action { | ||
| cont := pipeline.Action{Type: pipeline.Continue} | ||
| // The plugin's own LLM calls carry the sentinel; never recurse into them. | ||
| if pctx.Headers != nil && pctx.Headers.Get(sentinelHeader) != "" { |
There was a problem hiding this comment.
suggestion — This sentinel-header short-circuit is the one safety-critical branch: it prevents the plugin's own extract:code LLM call from recursing back through the hook. It's currently untested. Worth a unit test asserting OnRequest returns Continue and does not mutate the body when X-Context-Guru-LLM is set on the incoming headers.
There was a problem hiding this comment.
Added in #672: TestOnRequest_SentinelHeader_ShortCircuits asserts OnRequest returns Continue and does not mutate the body when X-Context-Guru-LLM is set on the incoming headers.
| kubectl -n "$NS" delete pod cgdrive --ignore-not-found >/dev/null; sleep 2 | ||
| kubectl -n "$NS" run cgdrive --restart=Never --image=curlimages/curl:8.10.1 \ | ||
| --overrides='{"spec":{"containers":[{"name":"c","image":"curlimages/curl:8.10.1","command":["sh","-c","curl -m 300 -s -X POST http://cg-finance-agent.team1.svc.cluster.local:8080/ -H '"'"'Content-Type: application/json'"'"' --data @/data/drive.json"],"volumeMounts":[{"name":"d","mountPath":"/data"}]}],"volumes":[{"name":"d","configMap":{"name":"cg-drive"}}]}}' >/dev/null | ||
| while [ "$(kubectl -n "$NS" get pod cgdrive -o jsonpath='{.status.phase}')" = "Running" ] || \ |
There was a problem hiding this comment.
suggestion — This poll loop continues only while the cgdrive pod phase is Running/Pending. If the pod goes Failed/Error (or is never scheduled), the loop exits and line 83 pipes the crashed pod's logs into json.loads, which throws a confusing Python traceback instead of a clear failure message. Consider breaking out explicitly on Failed/Error/Unknown and printing a readable error + kubectl describe.
There was a problem hiding this comment.
Fixed in #672. The poll loop now breaks explicitly on terminal phases (Failed/Unknown) with a readable error + kubectl describe (last 25 lines) + logs and a non-zero exit, so a crashed/unschedulable pod no longer pipes non-JSON into json.loads.
| # deliberately NO kagenti.io/inject -> the operator webhook does not inject a sidecar | ||
| spec: | ||
| containers: | ||
| - name: agent |
There was a problem hiding this comment.
suggestion — Neither container (agent, authbridge-proxy) declares resources requests/limits. Acceptable for a local kind demo, but since this doubles as a copy-paste template, modest requests/limits would make it a better starting point for real deployments.
There was a problem hiding this comment.
Added in #672 — both containers now declare modest resources requests/limits (agent 100m/128Mi → 1/512Mi, sidecar 50m/64Mi → 500m/256Mi) so the manifest is a better copy-paste template.
huang195
left a comment
huang195
left a comment
There was a problem hiding this comment.
Summary
Welcome, and a genuinely strong first contribution. 👏 plugin.go is clean and thoughtful: a sentinel-header recursion guard on its own LLM calls, path-gating, fail-open on engine hiccups (the right call for a compaction plugin — never break inference), a shared-but-synchronized pipe/store, per-session state keyed by A2A contextId, and proper Capabilities (WritesBody single-slot, mutually exclusive with SPARC; RequiresAny: [inference-parser]). Tests cover the meaningful paths — chain-ordering enforcement, Configure defaults/reject, real-engine compaction via SetBody, and the pass-throughs. Dockerfiles are a SHA-pinned Go 1.26 bump with an accurate "stays pure-Go / CGO_ENABLED=0" note, and the finance-mcp edit legitimately adds two large-output demo tools for context-guru to compact.
Shape / architecture note. context-guru embeds its engine in-process (linked Go library), which is the opposite of SPARC's split (SPARC ships a separate sparc-service Python sidecar the plugin calls over HTTP). That in-process choice is what pulls the full engine stack — maximhq/bifrost/core, bytedance/sonic, tiktoken-go, tree-sitter + ~15 go-sitter-forest grammars (dormant behind the cg_skeleton tag), starlark — into every default authbridge-proxy/authbridge-envoy build (~40 modules). It's pure-Go so no cgo, but it's a real footprint jump for the core binaries. Worth a conscious maintainer decision — see the inline note on opt-in vs opt-out.
Supply-chain notes (non-blocking): google.golang.org/protobuf is pinned to a pre-release pseudo-version (v1.36.12-0.20260120151049-f2248ac996af) in place of stable v1.36.11 — almost certainly forced transitively by bifrost/context-guru; worth confirming it can resolve to a stable tag. github.com/kagenti/context-guru is on an untagged pseudo-version (expected for a new companion module — pin to a release tag once cut). The go directive is an exact patch (go 1.26.4); go 1.26 + toolchain go1.26.4 is the more idiomatic split.
No blocking issues — two inline suggestions below. Nice work.
Author: OsherElhadad (first-time contributor — reviewed in full: deps, Dockerfiles, shell, k8s, plugin, tests; no secrets/agent-config/malware, DCO signed, CI green).
Assisted-By: Claude Code
| @@ -0,0 +1,5 @@ | |||
| //go:build !exclude_plugin_contextguru | |||
There was a problem hiding this comment.
suggestion — Registered opt-out (!exclude_plugin_contextguru), so context-guru is compiled into every authbridge-proxy / authbridge-envoy build by default. Unlike the tiny sibling plugins (static-inject, litellm-budget-track), the embedded engine pulls a large transitive set (bifrost/core + fasthttp, bytedance/sonic (+asm), tiktoken-go, tree-sitter/go-tree-sitter + ~15 go-sitter-forest grammars, starlark). It's pure-Go (CGO off; tree-sitter behind the unset cg_skeleton tag), so no cgo — but binary size / build time / dependency attack-surface of the core proxies grow for a feature not every deployment uses.
Consider making it opt-in (a positive build tag), or splitting the engine out-of-process the way SPARC does with sparc-service. Non-blocking design call — flagging so it's a conscious choice rather than incidental.
There was a problem hiding this comment.
Agreed — made it opt-IN in #672 (//go:build include_plugin_contextguru), so context-guru is no longer compiled into the default authbridge-proxy/authbridge-envoy binaries. Measured: default proxy 29 MB vs 45 MB with the tag (≈16 MB), 0 vs 312 context-guru symbols. run.sh and the demo Dockerfile build with -tags include_plugin_contextguru; documented in authbridge/CLAUDE.md as the deliberate exception to the exclude_plugin_* convention. (The full out-of-process split like sparc-service remains the larger option, noted as deferred.)
| AB="$(cd "$HERE/../.." && pwd)" # authbridge/ | ||
| FS="$AB/demos/finance-sparc" | ||
| OLLAMA_HOST_URL="${OLLAMA_HOST_URL:-http://127.0.0.1:11434}" # host Ollama, as seen from THIS machine | ||
| : "${CG_MODEL_BASE:=https://ete-litellm.ai-models.vpc-int.res.ibm.com}" |
There was a problem hiding this comment.
suggestion — The default CG_MODEL_BASE hardcodes an internal IBM VPC endpoint (https://ete-litellm.ai-models.vpc-int.res.ibm.com) in a public repo: it isn't reachable by external users and it leaks internal infra naming. Suggest a neutral placeholder (e.g. an OpenAI-compatible example URL) or leaving it unset so setup() fails loudly with guidance — mirrors how CG_MODEL_KEY is already required via : "${CG_MODEL_KEY:?...}".
There was a problem hiding this comment.
Fixed in #672 — the internal IBM endpoint is gone from both run.sh and agent.yaml. CG_MODEL_BASE is now required (fail-loud like CG_MODEL_KEY) and wired into the deployment via kubectl set env; agent.yaml ships a neutral https://api.openai.com placeholder. No real endpoint lands in the repo.
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
Follow-up to the merged context-guru plugin (rossoctl#660), addressing the non-blocking review comments from @mrsabath and @huang195. Plugin (authlib/plugins/contextguru): - OnRequest now skips compaction when there is no session identity instead of falling back to an empty ("") engine-store key, which would bleed per-session compaction state across unrelated callers. - Configure fails loud when exactly one of model.base_url / model.model is set (previously it silently degraded to deterministic — a typo'd or empty ${VAR} expansion would disable the cheap model with no error). - Add unit tests for the sentinel-header recursion guard (the one safety-critical branch) and the no-session skip. Build footprint: - Make context-guru opt-IN (//go:build include_plugin_contextguru) rather than opt-out. Its embedded engine pulls a large transitive set (bifrost/core, tiktoken-go, tree-sitter grammars, starlark), so the default authbridge-proxy/-envoy binaries stay lean (~16 MB smaller) and only pay for it when built with -tags include_plugin_contextguru. Documented in authbridge/CLAUDE.md as the deliberate exception to the exclude_plugin_* convention. Demo (demos/context-guru): - Remove the hardcoded internal IBM VPC endpoint from run.sh and agent.yaml; require CG_MODEL_BASE (fail-loud like CG_MODEL_KEY) and wire it into the deployment via kubectl set env, so no real endpoint lands in the repo. agent.yaml ships a neutral placeholder. - Harden the drive() poll loop: break on terminal pod phases (Failed/Unknown) with a readable error + describe/logs instead of piping crashed-pod logs into json.loads. - Add modest resources requests/limits to both demo containers so the manifest doubles as a usable template. - run.sh builds with -tags include_plugin_contextguru for the opt-in plugin. Supply-chain / hardening: - Digest-pin the envoy:v1.37.1 builder stage in the envoy Dockerfile (parity with the other stages). - Digest-pin alpine:3.20 in run.sh's generated demo Dockerfile. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Osher-Elhadad <Osher.Elhadad@ibm.com> Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
|
Thanks again @mrsabath and @huang195 for the thorough review. I've addressed all of the feedback in a follow-up PR: #672. Summary of what changed:
Two go.mod nits ( |
…lowups Fix: Address context-guru plugin review feedback (#660)
4 participants
Summary
Adds context-guru as an in-process AuthBridge outbound plugin that compacts an
agent's LLM request context (tool outputs) before it is forwarded upstream, plus a
runnable end-to-end demo on Kagenti. context-guru
(
github.com/kagenti/context-guru) reduces token cost deterministically (dedup,collapse, extract head/tail) and via an LLM-backed
extract:codecomponent.Relates to epic #659 and issue #641.
What's in it
authlib/plugins/contextguru/—OnRequesthands the outbound/v1/chat/completions(or/v1/messages) body toapply.BodyWithModeland, if theengine rewrote it,
SetBodys the compacted body.OnResponseis a pass-through in v1.Declares
WritesBody+RequiresAny:[inference-parser]. Logs every compaction(per-request summary at INFO, per-component at DEBUG) via a slog emitter.
cmd/authbridge-{proxy,envoy}/plugins_contextguru.go(build-tag gated,like every other plugin).
paths+ optionalmodel(static cheap model forextract:code, OpenAI-wire) +engine(context-guru's native config passed toconfig.LoadBytes). Model key is injected from a Secret via${CG_MODEL_KEY}.demos/context-guru/— a finance agent audits three transactions; only one has aplanted duplicate-settlement anomaly buried in ~18K tokens of audit-log noise.
run.shbuilds the plugin image, loads it + an enhanced
finance-mcp, and drives the audit inthree modes.
get_transaction_audit,get_customer_ledger) so the accumulated context is realistically large.Result (validated on a live Kagenti kind cluster, real agent, real Ollama)
Same agent, same
llama3.2model pinned to a 12,288-token window, same task:context-guru is the only variable that flips the answer from wrong to right. AuthBridge logs:
How to run
See
authbridge/demos/context-guru/README.mdfor the architecture diagram, configreference, and per-mode expected output.
Not in this integration (deferred by design)
OnResponseis a pass-through. Compaction isone-way in v1;
marker_mode: summaryleaves a⟪cg⟫breadcrumb but nothing restores it.The loop (resolve
<<cg:HASH>>from the Store → re-invoke upstream) is feasible inOnResponsevia a self-issuedllmclientcall (as SPARC/ibac do) — deferred to a follow-up.WritesBodyplugin, so it ismutually exclusive with
sparcon the outbound chain (the pipeline refuses to buildwith two). A deployment runs one or the other.
WritesBodyplugin forces SSE responses to buffer(≤1 MB, non-incremental) whenever context-guru is enabled — identical to SPARC. Request-side
compaction (the whole point here) is unaffected; response-side streaming with mutation is a
separate framework project.
Storeis in-memory per-pod. Cross-turn reuseand (future) restoration won't span replicas without an external Store behind the same interface.
Concerns for kagenti-extensions owners
go 1.26.4, soauthlib/go.mod, allcmd/*/go.mod, andgo.workwere bumped. CI readsgo-version-file: authbridge/authlib/go.mod, so all jobs now build on 1.26.4. Confirm yourrunners/base images support it (the Dockerfiles were moved to
golang:1.26-alpine, re-pinnedby digest).
authlib(tree-sitter grammars, starlark, bifrost→AWS SDK, otel). The default build ispure-Go (
CGO_ENABLED=0; skeleton's cgo is behind thecg_skeletontag), so none of thecgo ones compile — but they enlarge
go.sumand the dependency-review/Trivy surface. Considera thinner context-guru module boundary if this is a concern.
mainpseudo-version (v0.0.0-20260713…) since no releasetag exists yet. Bump to a tagged release once context-guru cuts one.
marker_modein v1.fullstashes every compacted original in the Store with no consumer(no expand loop yet) — pure memory overhead. The demo uses
summary(no stash). Prefersummary/offuntil restoration lands.extract:codecalls a configured model from withinOnRequest(blocking, gated by triggers) — the ibac pattern. It uses a standalone client thatbypasses the forward proxy, with a sentinel header as a reentrancy breaker.
cg-model-key+${CG_MODEL_KEY}expansion), never a ConfigMap. The:9094session API isunauthenticated and carries raw content — keep it cluster-internal.
Testing
authlib/plugins/contextguru/unit + build-integration tests (go test): pass; thebuild-integration test asserts
[inference-parser, context-guru]assembles and theRequiresAny ordering is enforced.
CGO_ENABLED=0 go build) against the publishedcontext-guru module (no replace): clean.
run.sh(results above),re-validated from scratch after switching to the published module.
Assisted-By: Claude (Anthropic AI) noreply@anthropic.com