From 5b7f89ff19b7b3cc996a91d5c5e2201ce1ee45f1 Mon Sep 17 00:00:00 2001 From: Osher-Elhadad Date: Wed, 15 Jul 2026 18:32:49 +0300 Subject: [PATCH] Fix: Address context-guru plugin review feedback (#660) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the merged context-guru plugin (#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) Signed-off-by: Osher-Elhadad Assisted-By: Claude (Anthropic AI) --- authbridge/CLAUDE.md | 5 ++- .../authlib/plugins/contextguru/plugin.go | 27 ++++++++++-- .../plugins/contextguru/plugin_test.go | 42 +++++++++++++++++++ authbridge/cmd/authbridge-envoy/Dockerfile | 2 +- .../authbridge-envoy/plugins_contextguru.go | 7 +++- .../authbridge-proxy/plugins_contextguru.go | 7 +++- authbridge/demos/context-guru/README.md | 29 +++++++++---- authbridge/demos/context-guru/k8s/agent.yaml | 12 +++++- authbridge/demos/context-guru/run.sh | 34 +++++++++++---- 9 files changed, 140 insertions(+), 25 deletions(-) diff --git a/authbridge/CLAUDE.md b/authbridge/CLAUDE.md index 5574ebba1..e3139eaf5 100644 --- a/authbridge/CLAUDE.md +++ b/authbridge/CLAUDE.md @@ -14,7 +14,10 @@ binaries with shared auth logic in `authlib/`: a2a-parser, mcp-parser, inference-parser, opa, sparc, ibac, token-broker). **Every** plugin is excludable via `-tags exclude_plugin_` — one `plugins_.go` file per plugin, gated by `//go:build !exclude_plugin_`; - `main.go` imports no plugin package directly. + `main.go` imports no plugin package directly. **Exception:** `context-guru` is + **opt-IN** (`//go:build include_plugin_contextguru`, not compiled by default), + because its embedded engine pulls a large transitive dependency set; build with + `-tags include_plugin_contextguru` to link it in. - `cmd/authbridge-envoy/` — envoy-sidecar mode. ext_proc gRPC server hooked into Envoy. Full plugin set. The `exclude_plugin_ibac` tag applies here too. - `authbridge-lite` (**image, not a separate binary**) — `cmd/authbridge-proxy` diff --git a/authbridge/authlib/plugins/contextguru/plugin.go b/authbridge/authlib/plugins/contextguru/plugin.go index 1c20e39c0..c719c26f8 100644 --- a/authbridge/authlib/plugins/contextguru/plugin.go +++ b/authbridge/authlib/plugins/contextguru/plugin.go @@ -199,7 +199,16 @@ func (p *ContextGuru) Configure(raw json.RawMessage) error { if m.MaxTokens > 0 { p.modelMax = m.MaxTokens } - if m.BaseURL != "" && m.Model != "" { + switch { + case m.BaseURL == "" && m.Model == "": + // Empty model block → no static model; LLM components degrade to + // deterministic. (Common when the config is present but the operator + // hasn't wired an endpoint yet.) + case m.BaseURL == "" || m.Model == "": + // Exactly one set is almost always a typo/empty ${VAR} expansion that + // would silently disable the cheap model. Fail loud instead. + return fmt.Errorf("context-guru model config: base_url and model must both be set (got base_url=%q model=%q)", m.BaseURL, m.Model) + default: p.static = cgModel{ c: llmclient.New(llmclient.Options{ Endpoint: m.BaseURL, @@ -227,19 +236,27 @@ func (p *ContextGuru) OnRequest(ctx context.Context, pctx *pipeline.Context) pip if p.pipe == nil || len(pctx.Body) == 0 || !p.gated(pctx.Path) { return cont } + // The engine Store is keyed by session for per-caller isolation. With no + // session identity every such request would share one empty key and bleed + // compaction state across unrelated callers, so skip compaction entirely. + sid := sessionID(pctx) + if sid == "" { + slog.Debug("context-guru skipped request with no session identity", "path", pctx.Path) + return cont + } provider := providerFor(pctx.Path) models := cgcomponents.ModelSpec{ Static: p.static, Incoming: p.incomingModel(pctx, provider), } before := len(pctx.Body) - out, changed := apply.BodyWithModel(ctx, p.pipe, p.store, provider, pctx.Body, sessionID(pctx), false, models) + out, changed := apply.BodyWithModel(ctx, p.pipe, p.store, provider, pctx.Body, sid, false, models) if changed && len(out) > 0 { pctx.SetBody(out) // Per-request byte-level view (the engine's token-level view is logged by // logEmitter.Run). The framework also emits a body-mutation session event. slog.Info("context-guru rewrote request body", - "provider", provider, "path", pctx.Path, "session", sessionID(pctx), + "provider", provider, "path", pctx.Path, "session", sid, "bytesBefore", before, "bytesAfter", len(out), "pctSaved", pct(before, len(out))) } return cont @@ -316,7 +333,9 @@ func modelFromBody(body []byte) string { } // sessionID keys per-session engine state to the conversation: the A2A contextId -// when present, else the caller subject for multi-tenant isolation. +// when present, else the caller subject for multi-tenant isolation. Returns "" +// when neither is available; OnRequest treats that as "skip compaction" rather +// than sharing one store key across unrelated callers. func sessionID(pctx *pipeline.Context) string { if pctx.Session != nil && pctx.Session.ID != "" { return pctx.Session.ID diff --git a/authbridge/authlib/plugins/contextguru/plugin_test.go b/authbridge/authlib/plugins/contextguru/plugin_test.go index 5aac91523..a0875e9c3 100644 --- a/authbridge/authlib/plugins/contextguru/plugin_test.go +++ b/authbridge/authlib/plugins/contextguru/plugin_test.go @@ -117,6 +117,48 @@ func TestOnRequest_CompactsLargeToolOutput(t *testing.T) { } } +// TestOnRequest_SentinelHeader_ShortCircuits covers the one safety-critical +// branch: the plugin's own extract:code LLM call carries the sentinel, and if it +// ever transits this forward proxy OnRequest must bail before touching the body — +// otherwise the compaction call recurses into itself. +func TestOnRequest_SentinelHeader_ShortCircuits(t *testing.T) { + p := configured(t, collapseEngine) + body := chatBody(bigToolOutput()) + pctx := inferencePctx("/v1/chat/completions", body) + pctx.Headers.Set(sentinelHeader, "1") + if act := invokeReq(p, pctx); act.Type != pipeline.Continue { + t.Fatalf("expected Continue, got %v", act.Type) + } + if pctx.BodyMutated() { + t.Fatal("sentinel header must short-circuit before any compaction") + } +} + +// TestOnRequest_NoSession_Skips verifies that a request with neither session nor +// identity is passed through untouched, so sessionless callers can't share one +// empty engine-store key. +func TestOnRequest_NoSession_Skips(t *testing.T) { + p := configured(t, collapseEngine) + body := chatBody(bigToolOutput()) + pctx := inferencePctx("/v1/chat/completions", body) + pctx.Session = nil // and Identity is nil → sessionID == "" + if act := invokeReq(p, pctx); act.Type != pipeline.Continue { + t.Fatalf("expected Continue, got %v", act.Type) + } + if pctx.BodyMutated() { + t.Fatal("no session identity must skip compaction (store-key isolation)") + } +} + +func TestConfigure_RejectsPartialModel(t *testing.T) { + if err := New().Configure(json.RawMessage(`{"model":{"base_url":"http://x"}}`)); err == nil { + t.Fatal("expected error when model.model is unset but base_url is set") + } + if err := New().Configure(json.RawMessage(`{"model":{"model":"gpt-4o-mini"}}`)); err == nil { + t.Fatal("expected error when model.base_url is unset but model is set") + } +} + func TestOnRequest_EmptyBody_PassThrough(t *testing.T) { p := configured(t, collapseEngine) pctx := inferencePctx("/v1/chat/completions", nil) diff --git a/authbridge/cmd/authbridge-envoy/Dockerfile b/authbridge/cmd/authbridge-envoy/Dockerfile index ce583ae9d..40c343b89 100644 --- a/authbridge/cmd/authbridge-envoy/Dockerfile +++ b/authbridge/cmd/authbridge-envoy/Dockerfile @@ -27,7 +27,7 @@ ENV GOWORK=off RUN cd cmd/authbridge-envoy && CGO_ENABLED=0 GOOS=linux go build -tags "${GO_BUILD_TAGS}" -ldflags="-s -w" -o /authbridge-envoy . # Stage 2: Get Envoy binary. Already stripped upstream; nothing to do. -FROM docker.io/envoyproxy/envoy:v1.37.1 AS envoy-source +FROM docker.io/envoyproxy/envoy:v1.37.1@sha256:29496a88fba9c4c9cdef4afe8fec70f536c5ba111b1c2bddbc5436b091ceca33 AS envoy-source # Stage 3: Runtime — UBI9-micro (glibc-compatible, ~24 MB, has bash) FROM registry.access.redhat.com/ubi9/ubi-micro@sha256:2173487b3b72b1a7b11edc908e9bbf1726f9df46a4f78fd6d19a2bab0a701f38 diff --git a/authbridge/cmd/authbridge-envoy/plugins_contextguru.go b/authbridge/cmd/authbridge-envoy/plugins_contextguru.go index f5bfb5785..cc01dcf30 100644 --- a/authbridge/cmd/authbridge-envoy/plugins_contextguru.go +++ b/authbridge/cmd/authbridge-envoy/plugins_contextguru.go @@ -1,5 +1,10 @@ -//go:build !exclude_plugin_contextguru +//go:build include_plugin_contextguru +// context-guru is opt-IN (positive build tag), unlike the other plugins which are +// compiled in by default and dropped via exclude_plugin_*. Its embedded engine +// pulls a large transitive set (bifrost/core, tiktoken-go, tree-sitter grammars, +// starlark), so it is kept out of the default authbridge-proxy/-envoy binaries and +// linked only when built with -tags include_plugin_contextguru. package main import _ "github.com/kagenti/kagenti-extensions/authbridge/authlib/plugins/contextguru" diff --git a/authbridge/cmd/authbridge-proxy/plugins_contextguru.go b/authbridge/cmd/authbridge-proxy/plugins_contextguru.go index f5bfb5785..cc01dcf30 100644 --- a/authbridge/cmd/authbridge-proxy/plugins_contextguru.go +++ b/authbridge/cmd/authbridge-proxy/plugins_contextguru.go @@ -1,5 +1,10 @@ -//go:build !exclude_plugin_contextguru +//go:build include_plugin_contextguru +// context-guru is opt-IN (positive build tag), unlike the other plugins which are +// compiled in by default and dropped via exclude_plugin_*. Its embedded engine +// pulls a large transitive set (bifrost/core, tiktoken-go, tree-sitter grammars, +// starlark), so it is kept out of the default authbridge-proxy/-envoy binaries and +// linked only when built with -tags include_plugin_contextguru. package main import _ "github.com/kagenti/kagenti-extensions/authbridge/authlib/plugins/contextguru" diff --git a/authbridge/demos/context-guru/README.md b/authbridge/demos/context-guru/README.md index d2b18647d..ce8c2e5e2 100644 --- a/authbridge/demos/context-guru/README.md +++ b/authbridge/demos/context-guru/README.md @@ -91,19 +91,34 @@ And the observed answers: ## Run it ```bash -export CG_MODEL_KEY= # e.g. an OpenAI-compatible key +export CG_MODEL_KEY= # e.g. an OpenAI-compatible key +export CG_MODEL_BASE=https://api.openai.com # any OpenAI-wire endpoint ./run.sh all # setup + drive off, observe, enforce and print the comparison # or step by step: ./run.sh setup ./run.sh drive enforce # (or observe / off) ``` -`run.sh setup` builds the `authbridge-proxy` image **with the context-guru plugin**, -loads it + the enhanced `finance-mcp` into the `kagenti` kind cluster, creates a -12,288-token-window Ollama model (`llama3.2-ctx12k`) so the raw request truncates, -and deploys the agent + sidecar. `run.sh drive ` flips `on_error`, restarts, -drives the audit, and prints the agent's answer + the byte/token gain from the -sidecar session API (`:9094`). +`run.sh setup` builds the `authbridge-proxy` image **with the context-guru plugin** +(`-tags include_plugin_contextguru` — see *Build integration* below), loads it + the +enhanced `finance-mcp` into the `kagenti` kind cluster, creates a 12,288-token-window +Ollama model (`llama3.2-ctx12k`) so the raw request truncates, and deploys the agent ++ sidecar. `run.sh drive ` flips `on_error`, restarts, drives the audit, and +prints the agent's answer + the byte/token gain from the sidecar session API (`:9094`). + +## Build integration + +context-guru is **opt-in**: unlike the other plugins (compiled in by default, +dropped via `-tags exclude_plugin_*`), it is linked only when the binary is built +with `-tags include_plugin_contextguru`. Its embedded engine pulls a large +transitive dependency set (bifrost/core, tiktoken-go, tree-sitter grammars, +starlark), so the default `authbridge-proxy`/`authbridge-envoy` binaries stay lean +and a deployment that doesn't want compaction never pays for it. + +```bash +cd authbridge && podman build -f cmd/authbridge-proxy/Dockerfile \ + --build-arg GO_BUILD_TAGS=include_plugin_contextguru -t authbridge-cg:latest . +``` ## Configuration reference diff --git a/authbridge/demos/context-guru/k8s/agent.yaml b/authbridge/demos/context-guru/k8s/agent.yaml index d211bf77e..554cbf460 100644 --- a/authbridge/demos/context-guru/k8s/agent.yaml +++ b/authbridge/demos/context-guru/k8s/agent.yaml @@ -39,17 +39,25 @@ spec: - { name: FINANCE_MCP_URL, value: "http://finance-mcp.team1.svc.cluster.local:8888" } - { name: AGENT_PUBLIC_URL, value: "http://cg-finance-agent.team1.svc.cluster.local:8080/" } volumeMounts: [{ mountPath: /shared, name: shared-data }] + resources: + requests: { cpu: "100m", memory: "128Mi" } + limits: { cpu: "1", memory: "512Mi" } - name: authbridge-proxy image: authbridge-cg:latest # authbridge-proxy built with the context-guru plugin (see run.sh) imagePullPolicy: IfNotPresent args: ["--config", "/etc/authbridge/config.yaml"] env: - - { name: CG_MODEL_BASE, value: "https://ete-litellm.ai-models.vpc-int.res.ibm.com" } - - { name: CG_MODEL_NAME, value: "claude-haiku-4-5" } # capable, cheap extract-code model + # Neutral placeholders — run.sh overrides these via `kubectl set env` + # from CG_MODEL_BASE / CG_MODEL_NAME so no real endpoint lands in the repo. + - { name: CG_MODEL_BASE, value: "https://api.openai.com" } + - { name: CG_MODEL_NAME, value: "gpt-4o-mini" } # capable, cheap extract-code model - name: CG_MODEL_KEY valueFrom: { secretKeyRef: { name: cg-model-key, key: key } } ports: [{ containerPort: 8080 }, { containerPort: 8081 }, { containerPort: 9094 }] volumeMounts: [{ mountPath: /etc/authbridge, name: ab-config }] + resources: + requests: { cpu: "50m", memory: "64Mi" } + limits: { cpu: "500m", memory: "256Mi" } volumes: - { name: shared-data, emptyDir: {} } - { name: ab-config, configMap: { name: cg-authbridge-config } } diff --git a/authbridge/demos/context-guru/run.sh b/authbridge/demos/context-guru/run.sh index 1a19e86ce..d5909c2b2 100755 --- a/authbridge/demos/context-guru/run.sh +++ b/authbridge/demos/context-guru/run.sh @@ -21,20 +21,20 @@ HERE="$(cd "$(dirname "$0")" && pwd)" 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}" -: "${CG_MODEL_NAME:=claude-haiku-4-5}" +: "${CG_MODEL_NAME:=gpt-4o-mini}" # any capable, cheap OpenAI-wire model CTX_MODEL="llama3.2-ctx12k" setup() { : "${CG_MODEL_KEY:?set CG_MODEL_KEY to the extract-code model API key}" + : "${CG_MODEL_BASE:?set CG_MODEL_BASE to an OpenAI-compatible endpoint for the extract-code model, e.g. https://api.openai.com}" local arch; arch=$(docker exec kagenti-control-plane uname -m | sed 's/aarch64/arm64/;s/x86_64/amd64/') - echo "==> build authbridge-proxy WITH the context-guru plugin ($arch, pure-Go) + image" + echo "==> build authbridge-proxy WITH the context-guru plugin ($arch, pure-Go, -tags include_plugin_contextguru) + image" ( cd "$AB" && GOTOOLCHAIN=auto GOOS=linux GOARCH="$arch" CGO_ENABLED=0 \ - go build -ldflags="-s -w" -o /tmp/authbridge-proxy-cg ./cmd/authbridge-proxy ) + go build -tags include_plugin_contextguru -ldflags="-s -w" -o /tmp/authbridge-proxy-cg ./cmd/authbridge-proxy ) local d=/tmp/cg-img; mkdir -p "$d"; cp /tmp/authbridge-proxy-cg "$d/authbridge-proxy" cat > "$d/Dockerfile" <<'DOCKER' -FROM alpine:3.20 +FROM alpine:3.20@sha256:beefdbd8a1da6d2915566fde36db9db0b524eb737fc57cd1367effd16dc0d06d RUN apk add --no-cache ca-certificates COPY authbridge-proxy /usr/local/bin/authbridge-proxy ENTRYPOINT ["/usr/local/bin/authbridge-proxy"] @@ -53,12 +53,18 @@ DOCKER -d "{\"model\":\"$CTX_MODEL\",\"from\":\"llama3.2:3b\",\"parameters\":{\"num_ctx\":12288}}" >/dev/null echo "==> secret + configmap + deploy agent + context-guru sidecar" - kubectl -n "$NS" create secret generic cg-model-key --from-literal=key="$CG_MODEL_KEY" \ + # Feed the key via a builtin (process substitution), not --from-literal, so it + # never appears in kubectl's argv / the process table on a shared host. + kubectl -n "$NS" create secret generic cg-model-key --from-file=key=<(printf %s "$CG_MODEL_KEY") \ --dry-run=client -o yaml | kubectl apply -f - kubectl -n "$NS" create configmap cg-authbridge-config --from-file=config.yaml="$HERE/k8s/authbridge-config.yaml" \ --dry-run=client -o yaml | kubectl apply -f - kubectl apply -f "$HERE/k8s/agent.yaml" kubectl -n "$NS" set env deploy/cg-finance-agent OLLAMA_MODEL="$CTX_MODEL" >/dev/null + # Point the extract-code model at the operator-provided endpoint (agent.yaml + # ships only a neutral placeholder; the real endpoint never lands in the repo). + kubectl -n "$NS" set env deploy/cg-finance-agent -c authbridge-proxy \ + CG_MODEL_BASE="$CG_MODEL_BASE" CG_MODEL_NAME="$CG_MODEL_NAME" >/dev/null kubectl -n "$NS" rollout status deploy/cg-finance-agent --timeout=120s } @@ -77,8 +83,20 @@ JSON 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" ] || \ - [ "$(kubectl -n "$NS" get pod cgdrive -o jsonpath='{.status.phase}')" = "Pending" ]; do sleep 5; done + # Wait for the driver pod to finish. Break explicitly on terminal failure so a + # crashed/unschedulable pod gives a readable error instead of piping non-JSON + # logs into json.loads below. + while :; do + phase=$(kubectl -n "$NS" get pod cgdrive -o jsonpath='{.status.phase}' 2>/dev/null || echo Unknown) + case "$phase" in + Succeeded) break ;; + Running|Pending) sleep 5 ;; + *) echo "!! cgdrive pod ended in phase=$phase — the audit did not complete:" + kubectl -n "$NS" describe pod cgdrive | tail -n 25 + kubectl -n "$NS" logs cgdrive 2>/dev/null || true + exit 1 ;; + esac + done echo "--- agent answer ($mode) ---" kubectl -n "$NS" logs cgdrive | python3 -c 'import sys,json;b=sys.stdin.read().split("EXIT=")[0].strip();d=json.loads(b);r=d.get("result",{});m=(r.get("status")or{}).get("message")or{};p=m.get("parts") or (r.get("artifacts") or [{}])[0].get("parts",[]);print(" ".join(x.get("text","") for x in p)[:600])' echo "--- context-guru gain ($mode) ---"