feat(eval): kb.effectiveness -- does surfaced knowledge change outcomes? - #537
Closed
tryeverything24 wants to merge 1 commit into
Closed
feat(eval): kb.effectiveness -- does surfaced knowledge change outcomes?#537tryeverything24 wants to merge 1 commit into
tryeverything24 wants to merge 1 commit into
Conversation
vouch can measure retrieval quality but not whether surfaced knowledge actually helps. adds a read-only, measurement-only signal: per approved artifact, correlate it being surfaced into a session's context pack with a coarse session outcome derived from the audit log (confirm/approve vs contradict/reject), reported with a 95% wilson interval and a verdict gated by statistical power -- useful/harmful render only once the interval clears the population baseline and --min-samples is met, otherwise unverified/insufficient. surfacing is recorded by kb.context (only when a session_id is given, same gating as the existing salience reflex) into a new context_surfacing table in index_db.SCHEMA -- a rebuildable derived cache, cleared by index_db.reset like every other cache table, not a knowledge write. kb.effectiveness itself never writes: no artifact, no audit event, no proposal. exposed as vouch health effectiveness / kb.effectiveness across all four registration sites (server.py, jsonl_server.py, capabilities.py METHODS, cli.py), plus the hot_memory exclusion table and the cli-mirror table in test_capabilities.py.
github-actions
Bot
added
docs
WalkthroughAdds the read-only ChangesEffectiveness measurement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ContextEndpoint
participant IndexDB
participant EffectivenessEndpoint
participant AuditLog
Client->>ContextEndpoint: request context with session_id
ContextEndpoint->>IndexDB: record surfaced artifact pairs
Client->>EffectivenessEndpoint: request effectiveness report
EffectivenessEndpoint->>IndexDB: read surfaced artifacts
EffectivenessEndpoint->>AuditLog: read session audit events
EffectivenessEndpoint-->>Client: return effectiveness report
Possibly related PRs
Suggested labels: 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
🧹 Nitpick comments (1)
src/vouch/index_db.py (1)
288-295: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
read_context_surfacingalways does a full, unbounded table scan.No
since/window parameter exists, so everykb.effectivenesscall reads every row ever recorded, regardless of--window. This table is written on everykb.contextcall carrying asession_id(a hot path), so it will grow much faster than the audit log and is never pruned outside of a fullindex_db.reset(). Worth adding an optionalsincefilter (WHERE surfaced_at >= ?) here and threading it through fromeffectiveness.compute()'ssincebefore this becomes a real cost.🤖 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 `@src/vouch/index_db.py` around lines 288 - 295, Update read_context_surfacing to accept an optional since timestamp and add a surfaced_at >= ? predicate when provided, while preserving the current oldest-first ordering and unfiltered behavior when omitted. Thread effectiveness.compute()'s since value into its call to read_context_surfacing so kb.effectiveness only reads rows within the requested window.
🤖 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 `@src/vouch/index_db.py`:
- Around line 262-285: Make record_context_surfacing best-effort by filtering
both kind and aid, and catching sqlite3.Error around database access, logging
the failure and returning without propagating it. In src/vouch/index_db.py lines
262-285 apply the root fix; src/vouch/jsonl_server.py lines 222-233 and
src/vouch/server.py lines 305-315 require no direct change once this function is
hardened, though a shared helper may replace their duplicated call-site
handling.
In `@tests/test_effectiveness.py`:
- Around line 265-271: Extend test_jsonl_effectiveness_handler to exercise the
JSONL handle_request() path rather than only HANDLERS["kb.effectiveness"],
asserting the successful {id, ok, result} envelope and the failure {id, ok:
false, error} envelope using an invalid min_samples value. Keep the existing
effectiveness payload assertions within the success result.
---
Nitpick comments:
In `@src/vouch/index_db.py`:
- Around line 288-295: Update read_context_surfacing to accept an optional since
timestamp and add a surfaced_at >= ? predicate when provided, while preserving
the current oldest-first ordering and unfiltered behavior when omitted. Thread
effectiveness.compute()'s since value into its call to read_context_surfacing so
kb.effectiveness only reads rows within the requested window.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1964f872-39a1-4650-87d8-a883e7e6d7d2
📒 Files selected for processing (11)
CHANGELOG.mdsrc/vouch/capabilities.pysrc/vouch/cli.pysrc/vouch/eval/__init__.pysrc/vouch/eval/effectiveness.pysrc/vouch/hot_memory.pysrc/vouch/index_db.pysrc/vouch/jsonl_server.pysrc/vouch/server.pytests/test_capabilities.pytests/test_effectiveness.py
Comment on lines
+262
to
+285
| def record_context_surfacing( | ||
| kb_dir: Path, *, session_id: str, items: Iterable[tuple[str, str]], | ||
| ) -> None: | ||
| """Log which artifacts were surfaced into a session's context pack. | ||
|
|
||
| Called from the read path (`kb.context`) only when a session_id is | ||
| given — same gating as the salience reflex. `items` is (kind, id) pairs. | ||
| Never called from `kb.effectiveness` itself, which only reads this | ||
| table: recording surfacing is a side effect of retrieval, not of | ||
| measurement. | ||
| """ | ||
| if not session_id: | ||
| return | ||
| pairs = [(kind, aid) for kind, aid in items if aid] | ||
| if not pairs: | ||
| return | ||
| ts = _dt.datetime.now(_dt.UTC).isoformat(timespec="seconds") | ||
| rows = [(session_id, kind, aid, ts) for kind, aid in pairs] | ||
| with open_db(kb_dir) as conn: | ||
| conn.executemany( | ||
| "INSERT INTO context_surfacing (session_id, kind, id, surfaced_at) " | ||
| "VALUES (?, ?, ?, ?)", | ||
| rows, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Context-surfacing telemetry write isn't actually "best-effort" — it can fail the primary kb.context response. record_context_surfacing is documented as a best-effort telemetry cache, not a knowledge write, but neither its own implementation nor either of its two call sites guards against it failing, so any error there (a kind=None NOT NULL violation, a locked db, etc.) currently turns an already-successfully-built context pack into a failed response.
src/vouch/index_db.py#L262-L285: filter onkind and aid(not justaid) and wrap theexecutemany/open_dbcall intry/except sqlite3.Error, logging and returning rather than propagating.src/vouch/jsonl_server.py#L222-L233: once the definition is hardened this site needs no change, but until then wrap theindex_db.record_context_surfacing(...)call here in try/except so a telemetry failure can't turn a successful context pack into aninternal_errorresponse.src/vouch/server.py#L305-L315: same fix as jsonl_server.py — this block is a byte-for-byte duplicate of the jsonl_server.py call site, so consider extracting a single shared helper (e.g. inindex_db.py) that bothkb_contextand_h_contextcall, rather than fixing the duplicated logic twice.
📍 Affects 3 files
src/vouch/index_db.py#L262-L285(this comment)src/vouch/jsonl_server.py#L222-L233src/vouch/server.py#L305-L315
🤖 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 `@src/vouch/index_db.py` around lines 262 - 285, Make record_context_surfacing
best-effort by filtering both kind and aid, and catching sqlite3.Error around
database access, logging the failure and returning without propagating it. In
src/vouch/index_db.py lines 262-285 apply the root fix;
src/vouch/jsonl_server.py lines 222-233 and src/vouch/server.py lines 305-315
require no direct change once this function is hardened, though a shared helper
may replace their duplicated call-site handling.
Comment on lines
+265
to
+271
| def test_jsonl_effectiveness_handler(store: KBStore, monkeypatch: pytest.MonkeyPatch) -> None: | ||
| monkeypatch.chdir(store.root) | ||
| from vouch.jsonl_server import HANDLERS | ||
|
|
||
| body = HANDLERS["kb.effectiveness"]({"window": "all", "min_samples": 1}) | ||
| assert body["sessions_considered"] == 0 | ||
| assert body["artifacts"] == [] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a JSONL envelope-shape assertion for kb.effectiveness.
This test exercises HANDLERS["kb.effectiveness"] directly and only checks the raw payload, not the {id, ok, result} envelope that handle_request() actually produces for JSONL clients (and the {id, ok: false, error} shape on failure, e.g. via a bad min_samples).
As per coding guidelines, "For each new kb.* method, add a test that asserts the JSONL envelope shape: {id, ok, result} on success and {id, ok: false, error} on failure."
✅ Proposed addition
def test_jsonl_effectiveness_handler(store: KBStore, monkeypatch: pytest.MonkeyPatch) -> None:
monkeypatch.chdir(store.root)
from vouch.jsonl_server import HANDLERS
body = HANDLERS["kb.effectiveness"]({"window": "all", "min_samples": 1})
assert body["sessions_considered"] == 0
assert body["artifacts"] == []
+
+
+def test_jsonl_effectiveness_envelope_shape(store: KBStore, monkeypatch: pytest.MonkeyPatch) -> None:
+ monkeypatch.chdir(store.root)
+ from vouch.jsonl_server import handle_request
+
+ ok = handle_request({"id": 1, "method": "kb.effectiveness", "params": {"window": "all"}})
+ assert ok["id"] == 1 and ok["ok"] is True and "result" in ok
+
+ bad = handle_request(
+ {"id": 2, "method": "kb.effectiveness", "params": {"min_samples": 0}}
+ )
+ assert bad["id"] == 2 and bad["ok"] is False and "error" in bad📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_jsonl_effectiveness_handler(store: KBStore, monkeypatch: pytest.MonkeyPatch) -> None: | |
| monkeypatch.chdir(store.root) | |
| from vouch.jsonl_server import HANDLERS | |
| body = HANDLERS["kb.effectiveness"]({"window": "all", "min_samples": 1}) | |
| assert body["sessions_considered"] == 0 | |
| assert body["artifacts"] == [] | |
| def test_jsonl_effectiveness_handler(store: KBStore, monkeypatch: pytest.MonkeyPatch) -> None: | |
| monkeypatch.chdir(store.root) | |
| from vouch.jsonl_server import HANDLERS | |
| body = HANDLERS["kb.effectiveness"]({"window": "all", "min_samples": 1}) | |
| assert body["sessions_considered"] == 0 | |
| assert body["artifacts"] == [] | |
| def test_jsonl_effectiveness_envelope_shape(store: KBStore, monkeypatch: pytest.MonkeyPatch) -> None: | |
| monkeypatch.chdir(store.root) | |
| from vouch.jsonl_server import handle_request | |
| ok = handle_request({"id": 1, "method": "kb.effectiveness", "params": {"window": "all"}}) | |
| assert ok["id"] == 1 and ok["ok"] is True and "result" in ok | |
| bad = handle_request( | |
| {"id": 2, "method": "kb.effectiveness", "params": {"min_samples": 0}} | |
| ) | |
| assert bad["id"] == 2 and bad["ok"] is False and "error" in bad |
🤖 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 `@tests/test_effectiveness.py` around lines 265 - 271, Extend
test_jsonl_effectiveness_handler to exercise the JSONL handle_request() path
rather than only HANDLERS["kb.effectiveness"], asserting the successful {id, ok,
result} envelope and the failure {id, ok: false, error} envelope using an
invalid min_samples value. Keep the existing effectiveness payload assertions
within the success result.
Source: Coding guidelines
Contributor
|
closing automatically: CodeRabbit requested changes and this pr has had no new commits for 2 days. the feedback still stands — push a fix and reopen this pr (or open a fresh one) and it will be reviewed again. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
what
closes #426. adds a read-only, measurement-only effectiveness signal:
vouch health effectiveness/kb.effectiveness.vouch can already measure retrieval quality (
vouch eval recall,embeddings evals) but nothing correlates "artifact X was surfaced into a
session's context pack" with what happened in that session. per approved
artifact, this reports the association between being surfaced and a
coarse session outcome derived from the audit log (a session whose events
lean
claim.confirm/proposal.*.approveis "good", leanclaim.contradict/proposal.*.rejectis "bad"; a tie, including nosignal at all, carries no evidence and is dropped from both sides rather
than folded into either bucket).
each artifact's good-outcome rate carries a 95% wilson interval computed
from (surfaced-and-good, surfaced-total). verdict gating is the point of
the feature:
useful/harmfulrender only once the interval clearsthe population baseline (the good-outcome rate across every session with
outcome signal in the window) and the sample meets
--min-samples;otherwise
unverified(interval straddles baseline) orinsufficient(not enough sessions yet). ranking is by the conservative (worst-plausible)
lift -- how far the interval's lower bound clears baseline -- so a wide,
lucky-looking interval doesn't outrank a tighter, more defensible one.
surfacing log (storage)
context-pack composition needed somewhere to live, so this adds a new
derived table to
index_db.SCHEMA:recorded by
kb.context(server.py+jsonl_server.py), only when asession_idis passed -- same gating the existing salience reflexalready uses, so there's no new consent surface. it's an internal derived
cache write via
index_db, never a knowledge-artifact write: noproposal, no audit event, no yaml. cleared by
index_db.resetlike everyother cache table (
reset's docstring calls out that, unlike the FTS/embedding tables, this one isn't re-derivable from durable files -- a
reset genuinely loses that surfacing history rather than rebuilding it,
which is an acceptable trade for a best-effort telemetry cache).
storage-migration risk: none. an existing
.vouch/directory picksup the new table the next time
index_db.open_dbruns its normalCREATE TABLE IF NOT EXISTSschema application -- no migration step, noversion bump, nothing to backfill (there's no history to backfill from;
the table starts recording from the first
kb.contextcall with asession_id after upgrade).
kb.effectivenessitself only reads:store.list_sessions(),audit.read_events(),index_db.read_context_surfacing(). it neverwrites an artifact, logs an audit event, or files a proposal -- covered
by
test_effectiveness_is_read_only.surfaces touched
new
kb.*method needs all four registration sites(
tests/test_capabilities.pyenforces this):kb_effectivenessinserver.py_h_effectivenessinjsonl_server.pycapabilities.pyMETHODSlistvouch health effectivenessincli.py(newhealthgroup;_CLI_MIRRORS["kb.effectiveness"] = "health effectiveness"since thedefault flat-command mirror rule doesn't apply)
also added to
hot_memory.HOT_MEMORY_EXCLUDED(aggregated ranking, not aclaim browse --
test_hot_memory_universal_coveragecatches any kb.*method that skips this classification).
tests
new
tests/test_effectiveness.py:--min-samples< 1rejected)
index_db.record_context_surfacing/read_context_surfacingroundtrip, and
index_db.resetclearing the new tablebelow
--min-samples, verdict staysinsufficient(5 good + 5 bad control sessions) plus an artifact surfaced only
alongside 8/8 good outcomes (verdict
useful,ci_lowclearsbaseline) and one surfaced only alongside 0/8 good outcomes (verdict
harmful,ci_highunder baseline) -- both n=8 margins are wideenough vs. baseline=0.5 that the assertions aren't flaky
context_surfacing,proposed/,sessions/all byte-identical before/aftercompute()kb.contextvia JSONL records surfacingonly when
session_idis passed, not otherwise--format json/--format text/ bad--window/bad
--min-samples) and the JSONL handler directlyvalidation
make checkgreen.Summary by CodeRabbit
New Features
vouch health effectiveness, JSONL, and MCP interfaces.Documentation