fix: Address code review on the tool-prune series

Correctness and privacy:

- snapshot.go: gjson's String() on an object or array returns that node's RAW
  JSON, so a structured error.type put response body content — anything the
  provider quoted from the request — into the unauthenticated session store,
  defeating the reason error.message is excluded. Now accepts only a JSON
  string or number. A test plants a credential in a structured value.
- toolprune: modelRates.rateFor reports whether a usable rate exists.
  set() ORs three fields, so a model configured with only a cache-read rate
  resolved as priced and then charged a cache-write request zero — vanishing
  from the total with no `requests unpriced` row.
- toolprune: the built-in per-model table now precedes the flat fallback. The
  flat fields are documented as covering models "absent from pricing", and a
  model in the table is not absent; one flat rate shadowing every per-model
  default reintroduced flat-rate mispricing, silently, and claimed to be
  operator-configured.
- toolprune: forcedToolChoice replaces forcedToolName. An object tool_choice
  naming nothing recognisable (Bedrock Converse nests it as tool.name) now
  declines to prune rather than reading it as "nothing forced" and risking
  removal of the one required tool.
- abctl: a response carrying a RequestID that fails to pair exactly — a retry,
  or a stream recorded twice — no longer falls through to the adjacency
  heuristic, where it could claim an unrelated earlier request. The same guard
  gates pricing, since a mismatched response supplies the wrong cache tier and
  the tiers are 12.5x apart.
- toolscan: PatchConfig writes via temp file + Sync + rename. os.WriteFile
  truncates in place, so a crash left a truncated config with no recovery copy,
  and the proxy's fsnotify reloader could observe the partial file.
- demo.go: writeDemoConfig keeps an existing demo.yaml. It runs before any port
  binds, so an unconditional write meant a --demo start that then failed on a
  port clash destroyed the operator's edits — including a prune list written by
  `abctl tools scan --write`, which the config's own comment recommends.

sparc declared WritesRequestBody but calls pctx.SetBody nowhere; the flag was
stale from the undirected capability and occupied the single request-mutator
slot, so [sparc, tool-prune] could not build. Dropped — which is the payoff
this series was arguing for, now pinned by a test.

Tests: real byte-exactness for the prune (reconstructing expected output from
the original bytes, covering first/middle/last element and validating with
encoding/json — the old test asserted only fragments and a shorter length, and
never removed a first or last element); tool_choice string forms; OpenAI-dialect
all-removed; and a reflection-driven clone check that fails if a future
slice/map capability is aliased.

Also: bytes.Contains on the scan hot path; names_unresolved distinguished from
no_configured_tool_present; an in-flight guard so the 2s refresh tick cannot
stack fetches against a 10s timeout; the dead crypto/rand fallback removed
(cannot fail as of Go 1.24); and docs corrected — WritesResponseBody added to
the capability snippet, the duplicated rate-derivation section removed, the
"ships with on_error: observe" claim replaced with the empty remove list that
is the actual guard, counters noted as resetting on hot-reload too, the
BodyAccess changelog line marked as since-removed, and the README's 20-25%
figure attributed to the traffic it was measured on.

Signed-off-by: Hai Huang <huang195@gmail.com>

Author: huang195

README.md

@@ -40,8 +40,9 @@ Watch an AI agent's traffic — its model, tool, and agent-to-agent calls — de
 ## Cut Claude Code token cost on your laptop
 
 Already using Claude Code? Cortex can strip the tool definitions your agent never
-calls out of every request — typically 20–25% of the prompt you pay for on each
-turn. Four steps, about two minutes:
+calls out of every request. On the traffic this was measured against that is
+20–25% of the prompt billed per turn; your share depends on how many of the
+tools you actually use. Four steps, about two minutes:
 **[Cut Claude Code token cost](./authbridge/docs/laptop-token-savings.md)**.
 
 ## Running on Kubernetes

authbridge/authlib/pipeline/bodydirection_test.go

@@ -163,3 +163,26 @@ func TestValidateCapabilities_Directional(t *testing.T) {
 		})
 	}
 }
+
+// TestValidateCapabilities_ResponseAndRequestMutatorsCoexist is the payoff the
+// directional split was arguing for. Before it, SPARC's undirected flag occupied
+// the only mutator slot, so a request-only mutator could not share a chain with
+// it even though the two write different bodies. Now the real in-tree shape —
+// parser, response mutator, request mutator — builds.
+func TestValidateCapabilities_ResponseAndRequestMutatorsCoexist(t *testing.T) {
+	err := validateCapabilities([]Plugin{
+		&stubPlugin{name: "inference-parser", caps: PluginCapabilities{ReadsBody: true}},
+		&stubPlugin{name: "sparc", caps: PluginCapabilities{WritesResponseBody: true}},
+		&stubPlugin{name: "tool-prune", caps: PluginCapabilities{WritesRequestBody: true}},
+	})
+	if err != nil {
+		t.Errorf("[parser, sparc, tool-prune] should build: %v", err)
+	}
+	// Two mutators on the SAME side are still rejected.
+	if err := validateCapabilities([]Plugin{
+		&stubPlugin{name: "sparc", caps: PluginCapabilities{WritesResponseBody: true}},
+		&stubPlugin{name: "cpex", caps: PluginCapabilities{WritesResponseBody: true}},
+	}); err == nil {
+		t.Error("two response mutators must still be rejected")
+	}
+}

authbridge/authlib/pipeline/errorkind_test.go

@@ -1,6 +1,9 @@
 package pipeline
 
-import "testing"
+import (
+	"strings"
+	"testing"
+)
 
 // TestUpstreamErrorKind: a bare "backend_error / 400" gives an operator nothing
 // to act on. The provider's own classification does — and it must be the
@@ -75,3 +78,36 @@ func TestDeriveError_PopulatesKindFrom4xxBody(t *testing.T) {
 		t.Errorf("bare 5xx = %+v, want backend_error/503 with empty message", bare)
 	}
 }
+
+// TestUpstreamErrorKind_RefusesStructuredValues is the privacy regression for a
+// leak the earlier test could not see: gjson's String() on an object or array
+// returns that node's RAW JSON. A provider (or a proxy in between) returning a
+// structured error.type therefore put response body content — including anything
+// quoted from the request — straight into the unauthenticated session store,
+// defeating the whole reason error.message is excluded.
+func TestUpstreamErrorKind_RefusesStructuredValues(t *testing.T) {
+	secret := "sk-live-DEADBEEF"
+	for _, body := range []string{
+		`{"error":{"type":{"secret":"` + secret + `","nested":true}}}`,
+		`{"error":{"type":["` + secret + `"]}}`,
+		`{"error":{"code":{"inner":"` + secret + `"}}}`,
+		`{"error":{"type":true}}`,
+		`{"error":{"type":null}}`,
+	} {
+		got := upstreamErrorKind([]byte(body))
+		if got != "" {
+			t.Errorf("structured value leaked %q from %s", got, body)
+		}
+		if strings.Contains(got, secret) {
+			t.Fatalf("CREDENTIAL LEAK: %q", got)
+		}
+	}
+	// A numeric code carries no payload and stays useful.
+	if got := upstreamErrorKind([]byte(`{"error":{"code":429}}`)); got != "429" {
+		t.Errorf("numeric code = %q, want 429", got)
+	}
+	// The normal string path is unaffected.
+	if got := upstreamErrorKind([]byte(`{"error":{"type":"rate_limit_error"}}`)); got != "rate_limit_error" {
+		t.Errorf("string type = %q", got)
+	}
+}

authbridge/authlib/pipeline/requestid.go

@@ -3,15 +3,8 @@ package pipeline
 import (
 	"crypto/rand"
 	"encoding/hex"
-	"strconv"
-	"sync/atomic"
 )
 
-// requestIDCounter is the fallback when crypto/rand is unavailable, so an id is
-// always produced rather than an empty string that would silently disable
-// pairing.
-var requestIDCounter atomic.Uint64
-
 // newRequestID returns a short, unique-per-process request identifier.
 //
 // Not a UUID on purpose: it exists to pair a request event with its response
@@ -20,9 +13,9 @@ var requestIDCounter atomic.Uint64
 // cryptographically meaningful.
 func newRequestID() string {
 	var b [6]byte
-	if _, err := rand.Read(b[:]); err != nil {
-		return "r" + strconv.FormatUint(requestIDCounter.Add(1), 36)
-	}
+	// crypto/rand.Read never returns an error as of Go 1.24 — it panics on an
+	// unusable system source instead — so there is no failure branch to write.
+	_, _ = rand.Read(b[:])
 	return hex.EncodeToString(b[:])
 }
 

authbridge/authlib/pipeline/snapshot.go

@@ -165,9 +165,15 @@ func upstreamErrorKind(body []byte) string {
 	if !gjson.ValidBytes(body) {
 		return ""
 	}
-	t := gjson.GetBytes(body, "error.type").String()
+	// Only accept a JSON string. gjson's String() on an object or array returns
+	// that node's RAW JSON, so {"error":{"type":{...}}} would put response body
+	// content — quoted request data, credentials — straight into the
+	// unauthenticated session store, defeating the whole point of excluding
+	// error.message. A numeric code is accepted because a number carries no
+	// payload; anything structured is refused.
+	t := stringOrNumber(gjson.GetBytes(body, "error.type"))
 	if t == "" {
-		t = gjson.GetBytes(body, "error.code").String()
+		t = stringOrNumber(gjson.GetBytes(body, "error.code"))
 	}
 	if t == "" {
 		return ""
@@ -177,3 +183,15 @@ func upstreamErrorKind(body []byte) string {
 	}
 	return t
 }
+
+// stringOrNumber returns the value only when the node is a JSON string or
+// number. Every other type — object, array, true/false, absent — yields "",
+// because String() on a container returns its raw JSON and that is body content.
+func stringOrNumber(r gjson.Result) string {
+	switch r.Type {
+	case gjson.String, gjson.Number:
+		return r.String()
+	default:
+		return ""
+	}
+}

authbridge/authlib/plugins/registry_capsclone_test.go

@@ -60,8 +60,41 @@ func TestCloneCatalog_PreservesEveryCapabilityField(t *testing.T) {
 	}
 }
 
-// TestCloneCatalog_DeepCopiesSlices: the clone must not alias the caller's
-// slices, or a mutation through /v1/plugins would reach into the registry.
+// TestCloneCatalog_DeepCopiesEveryReferenceField walks PluginCapabilities by
+// reflection and asserts that no field of a reference kind is aliased. The
+// struct copy in cloneCatalog is correct for today's two slices, but a future
+// map or slice capability would be silently shared with the registry — the same
+// class of bug the field-by-field copy had, which is why this is driven by the
+// struct rather than by a hand-written list.
+func TestCloneCatalog_DeepCopiesEveryReferenceField(t *testing.T) {
+	caps := nonZeroCaps(t)
+	in := []CatalogEntry{{Name: "probe", Capabilities: caps}}
+	out := cloneCatalog(in)
+
+	src := reflect.ValueOf(&in[0].Capabilities).Elem()
+	dst := reflect.ValueOf(&out[0].Capabilities).Elem()
+	for i := 0; i < src.NumField(); i++ {
+		name := src.Type().Field(i).Name
+		switch src.Field(i).Kind() {
+		case reflect.Slice:
+			if src.Field(i).Len() == 0 {
+				t.Fatalf("%s: nonZeroCaps left it empty, so aliasing cannot be detected", name)
+			}
+			if src.Field(i).UnsafePointer() == dst.Field(i).UnsafePointer() {
+				t.Errorf("%s aliases the registry's slice", name)
+			}
+		case reflect.Map, reflect.Pointer:
+			if src.Field(i).UnsafePointer() == dst.Field(i).UnsafePointer() {
+				t.Errorf("%s is a %s shared with the registry — cloneCatalog needs to copy it",
+					name, src.Field(i).Kind())
+			}
+		}
+	}
+}
+
+// TestCloneCatalog_DeepCopiesSlices keeps the concrete mutation check: the clone
+// must not alias the caller's slices, or a mutation through /v1/plugins would
+// reach into the registry.
 func TestCloneCatalog_DeepCopiesSlices(t *testing.T) {
 	in := []CatalogEntry{{
 		Name: "probe",

authbridge/authlib/plugins/sparc/plugin.go

@@ -209,10 +209,14 @@ func (p *SPARC) Capabilities() pipeline.PluginCapabilities {
 		// conversation + tool specs (both modes); mcp-parser provides the tool
 		// call (mcp mode). RequiresAny is a static "at least one" check; the
 		// per-mode runtime requirements are validated/handled below.
-		RequiresAny:        []string{"inference-parser", "mcp-parser"},
-		ReadsBody:          true,
-		WritesRequestBody:  true, // MCP result (mcp mode) / completion rewrite (inference mode)
-		WritesResponseBody: true, // respond.go rewrites the upstream response
+		RequiresAny: []string{"inference-parser", "mcp-parser"},
+		ReadsBody:   true,
+		// Response-only: SPARC rewrites the upstream response (respond.go), and
+		// calls pctx.SetBody nowhere. Declaring WritesRequestBody was carried over
+		// from the undirected flag and cost it the single request-mutator slot for
+		// nothing, so a chain like [sparc, tool-prune] could not build even though
+		// the two write different bodies.
+		WritesResponseBody: true,
 		Description:        "SPARC pre-tool reflection: blocks ungrounded/hallucinated tool calls.",
 	}
 }

authbridge/authlib/plugins/sparc/plugin_test.go

@@ -335,10 +335,22 @@ func TestInference_MCPModeOnResponseIsNoop(t *testing.T) {
 	}
 }
 
+// TestCapabilities pins SPARC as a RESPONSE-side mutator. It rewrites the
+// upstream response (respond.go) and calls pctx.SetBody nowhere, so declaring
+// WritesRequestBody was carried over from the undirected flag and cost it the
+// single request-mutator slot for nothing — a chain like [sparc, tool-prune]
+// could not build even though the two write different bodies.
 func TestCapabilities(t *testing.T) {
 	caps := NewSPARC().Capabilities()
-	if !caps.WritesRequestBody || !caps.ReadsBody {
-		t.Error("expected ReadsBody+WritesRequestBody")
+	if !caps.WritesResponseBody {
+		t.Error("expected WritesResponseBody — SPARC rewrites the response")
+	}
+	if caps.WritesRequestBody {
+		t.Error("must not declare WritesRequestBody: SPARC never calls pctx.SetBody, " +
+			"and the claim blocks any real request mutator from sharing the chain")
+	}
+	if !caps.Normalize().ReadsBody {
+		t.Error("a write flag must promote ReadsBody")
 	}
 	if len(caps.RequiresAny) == 0 {
 		t.Error("expected RequiresAny parsers")

authbridge/authlib/plugins/toolprune/metrics.go

@@ -119,7 +119,7 @@ func (m *metrics) observeSaving(tokens float64, t tier, usd float64, src rateSou
 // snapshot renders the counters as operator-facing metrics. Every derived row
 // carries the sample it was computed from, so a figure can never be read as
 // more certain than it is.
-func (m *metrics) snapshot(cfg *config) []pipeline.Metric {
+func (m *metrics) snapshot() []pipeline.Metric {
 	m.mu.Lock()
 	defer m.mu.Unlock()