feat(learning): add learning.facet_provenance — evidence behind a facet#4544
feat(learning): add learning.facet_provenance — evidence behind a facet#4544mysma-9403 wants to merge 3 commits into
Conversation
The learning domain exposes facets (list_facets / get_facet) with stability,
confidence and evidence_count, but not the evidence itself — the UI can say
"80% confident, from 12 observations" but not "which 12?". Facets already store
typed EvidenceRefs pointing back into the memory substrate (episodic entries,
tree topics, document chunks, emails, provider fields, tool calls); that
provenance was never surfaced.
Add learning.facet_provenance (class + key -> { found, key, evidence_count,
evidence[] }). Each evidence entry is { type, label, ...ref-specific ids } via
pure provenance_entry / provenance_entries helpers — no cross-store lookups, so
deterministic and cheap; the type + ids let a later iteration deep-link into the
source. The match is exhaustive over all nine EvidenceRef variants. Backs a
"what the assistant has learned about you" transparency panel (UI is a
follow-up); pairs with the existing pin/forget facet controls.
Tests: provenance_entry rendering per variant + order/length; schema shape;
controller counts updated to twelve.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new Changesfacet_provenance Controller
Estimated code review effort: 2 (Simple) | ~15 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 1
🧹 Nitpick comments (1)
src/openhuman/learning/schemas.rs (1)
496-554: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider testing the remaining
EvidenceRefvariants.
provenance_entry_renders_type_label_and_idsonly exercisesEpisodic,TreeTopic, andEmailMessage. Given the handler is variant-exhaustive by design, coveringEpisodicWindow,SourceSummary,DocumentChunk,Provider,ToolCall, andTreeSourceWeightwould catch regressions if a variant's fields or label format change.🤖 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/openhuman/learning/schemas.rs` around lines 496 - 554, The provenance_entry_renders_type_label_and_ids test only covers a few EvidenceRef variants, so add assertions for the remaining exhaustive variants handled by provenance_entry and provenance_entries. Expand the test to instantiate and verify the expected type/label/identifier fields for EpisodicWindow, SourceSummary, DocumentChunk, Provider, ToolCall, and TreeSourceWeight, using the existing EvidenceRef and provenance_entry symbols to keep the coverage aligned with the variant-specific formatting.
🤖 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/openhuman/learning/schemas.rs`:
- Around line 848-927: `provenance_entry` and `provenance_entries` are
business-logic rendering helpers, not RPC handlers, so they should not live in
`schemas.rs`. Move these functions into `ops.rs` under the `openhuman::learning`
module, keep their behavior unchanged, and update the `schemas.rs` caller sites
to import and use them from `ops.rs` so the canonical module split is preserved.
---
Nitpick comments:
In `@src/openhuman/learning/schemas.rs`:
- Around line 496-554: The provenance_entry_renders_type_label_and_ids test only
covers a few EvidenceRef variants, so add assertions for the remaining
exhaustive variants handled by provenance_entry and provenance_entries. Expand
the test to instantiate and verify the expected type/label/identifier fields for
EpisodicWindow, SourceSummary, DocumentChunk, Provider, ToolCall, and
TreeSourceWeight, using the existing EvidenceRef and provenance_entry symbols to
keep the coverage aligned with the variant-specific formatting.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b9a9296d-48bd-4500-9c6c-bcef4d6f1e22
📒 Files selected for processing (2)
src/openhuman/learning/README.mdsrc/openhuman/learning/schemas.rs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 520575ecd9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let facet = cache.get(&fk).map_err(|e| format!("get failed: {e:#}"))?; | ||
|
|
||
| let (found, evidence, evidence_count) = match &facet { | ||
| Some(f) => (true, provenance_entries(&f.evidence_refs), f.evidence_count), |
There was a problem hiding this comment.
When a facet has competing candidates for the same (class, key) but different values, the cached evidence_refs are not value-specific: stability_detector appends every candidate's evidence for that key before persisting the single winning value (src/openhuman/learning/stability_detector.rs:261-268). Rendering f.evidence_refs here therefore reports sources that may support a losing value (for example, a discarded verbose observation on a style/verbosity = terse facet) as provenance for the current facet, which makes the transparency UI cite contradictory evidence. Please either persist/return only refs for the winning value or include the value each ref supported.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. The refs are facet-key-scoped by the detector's storage model — stability_detector merges every candidate's evidence for a (class, key) before persisting the single winning value — so filtering to the winning value here isn't possible without also changing what the detector persists (per-value refs, or tagging each ref with the value it supported). That's a separate change to the learning storage model, out of scope for this rendering-only RPC. For now I've made the contract honest instead of overclaiming: the schema description + evidence field comment + helper doc now state the refs are key-scoped and may include observations that reinforced a prior/competing value, so the surface frames them as "observations for this facet," not proof of the current value. Happy to file a follow-up for value-scoped provenance if you'd like it.
There was a problem hiding this comment.
Confirming this thread from a fresh review: the finding is accurate — stability_detector::rebuild calls merge_evidence_refs (stability_detector.rs:265) over all candidates for a (class, key) before persisting the single winning value, so evidence_refs are key-scoped, not value-scoped. The author's resolution (making the schema description, the evidence field comment, and the helper doc all state the refs are key-scoped and may include observations that reinforced a prior/competing value) is the right call for a rendering-only RPC — value-scoped provenance is a separate change to what the detector persists. Agree with resolving as documented; a tracking issue for value-scoped provenance would be a nice follow-up.
There was a problem hiding this comment.
Thanks for the independent confirm. Agreed the honest-contract framing (schema description + evidence field comment + helper doc all stating the refs are key-scoped) is the right call for a rendering-only RPC — value-scoped provenance is a change to what stability_detector persists (per-value refs, or tagging each ref with the value it supported), out of scope here. Happy to file a tracking issue for value-scoped provenance so the follow-up is not lost — will do unless a maintainer prefers to fold it into the learning storage roadmap.
Addresses Codex review on tinyhumansai#4544: a facet's evidence_refs are merged by the stability detector across every candidate for a (class, key) before persisting the single winning value, so provenance can include observations that reinforced a prior/competing value — not proof of the current value. Make the RPC contract say so (schema description + `evidence` field comment + the provenance_entries doc) so the "what I know about you" surface frames it as "observations for this facet," not "evidence the value is X". Value-scoped provenance would need the detector to persist per-value refs — out of scope for this rendering-only change.
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
PR #4544 — feat(learning): add learning.facet_provenance — evidence behind a facet
Walkthrough
This PR adds a read-only learning.facet_provenance RPC to the learning domain. Given a (class, key) it looks up the facet in the ambient cache and renders its stored evidence_refs as UI-friendly { type, label, …ids } entries via two pure helpers (provenance_entry / provenance_entries), plus returns found, the resolved key, and the total evidence_count. It's a clean, well-scoped backend slice: the controller is wired through the registry (not ad-hoc branches), the match over EvidenceRef is exhaustive, and the schema docs are honest about the key-scoped (not value-scoped) semantics. The main gap is test coverage — the RPC handler and 6 of the 9 render arms are untested, which likely trips the ≥80% diff-coverage gate.
Changes
| File | Summary |
|---|---|
src/openhuman/learning/schemas.rs |
New learning_facet_provenance schema + handle_facet_provenance handler + pure provenance_entry/provenance_entries renderers; registry + schema list wired; controller-count tests bumped 11→12; one render test added. |
src/openhuman/learning/README.md |
Controller count 11→12; new method row documenting learning.facet_provenance. |
Actionable comments (2)
⚠️ Major
1. src/openhuman/learning/schemas.rs:858-936 / :1020-1056 — Handler + 6/9 render arms are untested; likely fails the diff-coverage gate
provenance_entry_renders_type_label_and_ids exercises only Episodic, TreeTopic, and EmailMessage. The other six arms (EpisodicWindow, SourceSummary, DocumentChunk, Provider, ToolCall, TreeSourceWeight) and the entire handle_facet_provenance body have zero coverage — no unit test drives the handler and no tests/json_rpc_e2e.rs case was added. The exhaustive match guarantees the arms compile, but not that each emits the intended type/label/ids. On the CI-Lite ≥80%-diff-coverage gate this block of new lines is a real risk. Extending the existing table-style test is cheap and closes both the coverage and correctness gap.
Suggested change:
// add to provenance_entry_renders_type_label_and_ids (or a new test):
let win = provenance_entry(&EvidenceRef::EpisodicWindow { from_id: 3, to_id: 7 });
assert_eq!(win["type"], "episodic_window");
assert_eq!(win["from_id"], 3);
assert_eq!(win["to_id"], 7);
let chunk = provenance_entry(&EvidenceRef::DocumentChunk {
source_id: "doc:1".into(), chunk_id: "c9".into(),
});
assert_eq!(chunk["type"], "document_chunk");
assert_eq!(chunk["chunk_id"], "c9");
let provider = provenance_entry(&EvidenceRef::Provider {
toolkit: "gmail".into(), connection_id: "conn1".into(), field: "timezone".into(),
});
assert_eq!(provider["type"], "provider");
assert_eq!(provider["label"], "gmail · timezone");
let tool = provenance_entry(&EvidenceRef::ToolCall {
tool_name: "web_search".into(), episodic_id: 5,
});
assert_eq!(tool["type"], "tool_call");
assert_eq!(tool["episodic_id"], 5);
let sw = provenance_entry(&EvidenceRef::TreeSourceWeight { window_label: "w1".into() });
assert_eq!(sw["type"], "tree_source_weight");
let sum = provenance_entry(&EvidenceRef::SourceSummary { summary_id: "s1".into() });
assert_eq!(sum["type"], "source_summary");Consider also a tests/json_rpc_e2e.rs case that dispatches learning.facet_provenance end-to-end (found + not-found) so the handler's param parsing and payload shape are covered.
💡 Nitpick / suggestion
2. src/openhuman/learning/schemas.rs:334 — evidence_count schema declares U64 but the field is i32
The output schema types evidence_count as TypeSchema::U64, while ProfileFacet::evidence_count is i32 (memory_store/unified/profile.rs:230) and is emitted raw (the None branch also yields an i32 0). It serializes fine either way, so this is cosmetic — but the declared type doesn't match the wire value. This mirrors an existing inconsistency in facet_to_json, so it's fine to leave; flagging for accuracy. If you want the schema to be truthful, TypeSchema::I64 matches the source type.
Nitpicks (0 additional)
Questions for the author (0)
Verified / looks good
- Controller correctly wired through the registry (
all_learning_registered_controllers) and schema list — no ad-hoccli.rs/jsonrpc.rsbranch. ✅ matchoverEvidenceRefis exhaustive across all nine variants, so a new variant can't silently fall through (compile error forces an update). ✅- Handler propagates errors as
Result<_, String>→RpcOutcomeconsistently with sibling handlers; missingclass/keyreturn clear errors. ✅ not-foundpath returnsfound=false, evidence=[], evidence_count=0rather than erroring — sensible for a transparency surface. ✅- Entry
tracing::debug!+ structuredlogline present; no secrets/PII logged (only the source count, not source contents). ✅ - README count (12) matches
all_learning_controller_schemas().len()and the count tests. ✅ - Codex P2 (provenance not filtered to the winning value) — verified accurate:
stability_detector::rebuild(stability_detector.rs:265) merges every candidate's evidence per(class,key)before persisting the winning value, soevidence_refsare key-scoped. The author's resolution — making the schema/field/helper docs explicitly say the refs are key-scoped rather than proof of the current value — is the right call for a rendering-only RPC; a value-scoped model is a separate storage change. Agree with leaving it as documented, ideally with a tracking issue.
Review posted as a comment — not approving; merge/approval is the maintainer's call.
| /// `type` discriminator, a short human-readable `label`, and the ref's own | ||
| /// identifier fields so a caller can deep-link into the source later. Pure — | ||
| /// no cross-store lookups — so it is cheap and deterministic. | ||
| fn provenance_entry(r: &crate::openhuman::learning::candidate::EvidenceRef) -> serde_json::Value { |
There was a problem hiding this comment.
Major (test coverage): provenance_entry_renders_type_label_and_ids covers only Episodic, TreeTopic, and EmailMessage. The other six EvidenceRef arms rendered here — EpisodicWindow, SourceSummary, DocumentChunk, Provider, ToolCall, TreeSourceWeight — plus the whole handle_facet_provenance body have no coverage (no unit test drives the handler, and no tests/json_rpc_e2e.rs case was added). The exhaustive match guarantees these arms compile but not that each emits the intended type/label/ids. On the CI-Lite ≥80%-diff-coverage gate this new block is a real risk. Extending the table-style test closes both gaps cheaply:
let provider = provenance_entry(&EvidenceRef::Provider {
toolkit: "gmail".into(), connection_id: "conn1".into(), field: "timezone".into(),
});
assert_eq!(provider["type"], "provider");
assert_eq!(provider["label"], "gmail · timezone");
let win = provenance_entry(&EvidenceRef::EpisodicWindow { from_id: 3, to_id: 7 });
assert_eq!(win["type"], "episodic_window");
assert_eq!(win["to_id"], 7);
// …DocumentChunk / ToolCall / TreeSourceWeight / SourceSummary similarlyAlso consider a json_rpc_e2e case dispatching learning.facet_provenance (found + not-found) so the handler's param parsing and payload shape are exercised end-to-end.
There was a problem hiding this comment.
Done in 2835666.
provenance_entry_renders_every_variant now asserts type/label/id fields for all nine EvidenceRef arms — added the six that were missing (EpisodicWindow, SourceSummary, DocumentChunk, Provider, ToolCall, TreeSourceWeight), including the exact provider / episodic_window rows you sketched.
For the handler body: I extracted a pure build_provenance_payload(fk, Option<&ProfileFacet>) out of handle_facet_provenance — the async fn now keeps only param-parse + get_cache()/cache.get() plumbing (which every other handler in this module leaves to integration coverage). The new build_provenance_payload_found_and_not_found unit test drives both branches directly: the not-found found=false/empty-evidence shape and the found path (payload keys + raw i32 evidence_count passthrough + ordered render of stored refs).
I went with the extracted-helper route rather than a json_rpc_e2e case on purpose: in that harness memory::global::client_if_ready() is not initialized for direct RPC, so learning.facet_provenance would return memory client not ready and exercise an error path instead of the found/not-found payload. The unit test verifies the same contract without that fragility.
| required: true, | ||
| }, | ||
| FieldSchema { | ||
| name: "evidence_count", |
There was a problem hiding this comment.
Nitpick (schema/type accuracy): evidence_count is declared TypeSchema::U64 here, but ProfileFacet::evidence_count is i32 (memory_store/unified/profile.rs:230) and is emitted raw (the not-found branch also yields an i32 0). It serializes fine either way, so this is cosmetic — but the declared type doesn't match the wire value. facet_to_json has the same existing inconsistency, so it's OK to leave; if you'd like the schema to be truthful, TypeSchema::I64 matches the source type.
There was a problem hiding this comment.
Fixed in 2835666 — evidence_count is now TypeSchema::I64, with a comment noting the wire value is ProfileFacet::evidence_count (an i32, emitted raw), so the declared type matches the source. Left facet_to_json untouched: it emits raw JSON with no declared FieldSchema, so there is no schema type to disagree with there.
…nce payload Address review on tinyhumansai#4544: - Extend provenance_entry test from 3 to all 9 EvidenceRef variants so the exhaustive match is verified to emit the intended type/label/id fields, not just compiled (closes the diff-coverage gap on the new render arms). - Extract pure build_provenance_payload() from handle_facet_provenance so the found / not-found payload shape (and raw i32 evidence_count passthrough) is unit-testable without a live FacetCache; the async handler keeps only the param-parse + cache plumbing. - Fix facet_provenance output schema: evidence_count is I64, not U64 — the wire value is ProfileFacet::evidence_count (an i32, emitted raw).
🤖 Council Merge Gate — APPROVE (merge blocked by branch protection)The model council review gate returned APPROVE (session However, Action needed (human): resolve the failing required checks / obtain the required review, then merge. A maintainer with admin rights may also merge explicitly. The PR is left OPEN. Posted automatically by the |
🤖 Council Merge Gate — APPROVE (merge blocked by branch protection)The model council review gate returned APPROVE (session However, Action needed (human): resolve the failing required checks / obtain the required review, then merge. A maintainer with admin rights may also merge explicitly. The PR is left OPEN. Posted automatically by the |
3 participants
What
A backend slice toward a "what the assistant has learned about you" transparency panel: a new
learning.facet_provenanceRPC that returns the evidence behind a single profile facet.Why
The learning domain already exposes facets (
learning.list_facets/learning.get_facet) withstability,confidence, andevidence_count— but not the evidence itself. So the UI can say "the assistant is 80% confident you prefer terse replies, from 12 observations" but can't answer "which 12?". The facets already store typedEvidenceRefs pointing back into the memory substrate (episodic entries, tree topics, document chunks, emails, provider fields, tool calls) — that provenance was just never surfaced. (Those refs are the ones hardened against duplicate accumulation in #4484.)Change
learning.facet_provenance(class + key →{ found, key, evidence_count, evidence[] }). Eachevidence[]entry is{ type, label, …ref-specific ids }, e.g.{ "type": "episodic", "label": "Episodic memory #42", "episodic_id": 42 }.provenance_entry/provenance_entrieshelpers map eachEvidenceRefvariant to a UI-friendly shape. No cross-store lookups — deterministic and cheap; thetype+ ids let a later iteration deep-link into the actual source. The match is exhaustive over all nineEvidenceRefvariants, so a new variant can't silently fall through.evidence_count(total reinforcing observations) is returned alongsideevidence[](the traceable, deduped refs) — the former can exceed the latter, which the panel can show as "learned from N observations, M traceable."Scope
Deliberately the Rust core + RPC — the input for the panel. The React screen (an "Intelligence → what I've learned" view rendering facets + their provenance, with per-facet pin/forget already available via the existing RPCs) is the natural next PR; keeping it out here keeps this reviewable and i18n-free. Happy to open a tracking issue for the UI if preferred.
Tests
provenance_entry_renders_type_label_and_ids— episodic / tree_topic / email variants render the righttype,label, and id fields;provenance_entriespreserves order and length.facet_provenance_schema_shape— required inputs (class,key) and output field names.cargo test --lib learning::schemasgreen locally.Summary by CodeRabbit
New Features
Documentation
Tests