Skip to content

feat(learning): surface facet provenance (cue_families, evidence_refs) in RPC - #5043

Merged
senamakel merged 1 commit into
tinyhumansai:mainfrom
mysma-9403:feat/learning-facet-provenance-fields
Jul 23, 2026
Merged

feat(learning): surface facet provenance (cue_families, evidence_refs) in RPC#5043
senamakel merged 1 commit into
tinyhumansai:mainfrom
mysma-9403:feat/learning-facet-provenance-fields

Conversation

@mysma-9403

@mysma-9403 mysma-9403 commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Surface the two provenance fields the learning store already persists — evidence_refs and cue_families — through the facet RPC serializer, which was silently dropping them at the boundary.
  • Additive-only: two keys added to facet_to_json, so every facet-returning controller (list_facets, get_facet, update/pin/unpin/forget) gains them at once.
  • Add a unit test asserting both fields round-trip.

Problem

facet_to_json is the single serializer feeding all facet-returning learning.* controllers. ProfileFacet persists (and row_to_facet hydrates) two provenance columns:

  • evidence_refs: Vec<EvidenceRef> (from evidence_refs_json) — the citations behind a facet.
  • cue_families: Option<HashMap<String, u32>> (from cue_families_json) — per-cue-family evidence counts written by the stability detector.

Both derive Serialize (EvidenceRef is a #[serde(tag = "type")] enum), are documented as first-class ProfileFacet state, and exist specifically as provenance — yet the serializer emits only 10 keys and drops both, so no RPC consumer can see the evidence behind a facet.

Solution

Add the two fields to the json! macro in facet_to_json:

"cue_families": f.cue_families,
"evidence_refs": f.evidence_refs,

The controller output schemas declare facet/facets as opaque TypeSchema::Json, so the schema contract is unchanged; the addition is purely additive metadata. No store or query changes.

Scope note: evidence_refs are keyed to a facet's (class, key), not to the specific winning value (the stability detector merges every candidate's evidence for a key before persisting the single value). This PR only stops the serializer from dropping already-persisted state; it does not change what the detector stores. Value-scoped provenance would be a separate, larger change.

Submission Checklist

  • Tests added or updated (happy path + at least one failure / edge case) — facet_to_json_includes_cue_families_and_evidence_refs builds a facet with populated and empty provenance and asserts both keys serialize correctly (present-and-populated + null/empty edge).
  • Diff coverage ≥ 80% — the two added serializer lines are exercised by the new test. cargo test -p openhuman --lib learning::schemas passes.
  • Coverage matrix updated — N/A: additive read-only field exposure, no new controller/feature row.
  • All affected feature IDs listed under ## RelatedN/A.
  • No new external network dependencies introduced.
  • Manual smoke checklist updated — N/A: does not touch a release-cut surface.
  • Linked issue closed via Closes #NNN — no existing issue; found by inspection.

Impact

  • Desktop/CLI: learning.list_facets / get_facet (and the mutating controllers' echoed facet) now include cue_families and evidence_refs, enabling a transparency/provenance UI to cite the evidence behind a learned facet without a second round-trip. Purely additive; no behaviour change for existing consumers.

Related

  • Closes:
  • Follow-up PR(s)/TODOs: value-scoped provenance (persist/return refs per winning value) — separate change to the learning storage model.

AI Authored PR Metadata

Linear Issue

  • Key: N/A
  • URL: N/A

Commit & Branch

  • Branch: feat/learning-facet-provenance-fields
  • Commit SHA: e4f6889af

Validation Run

  • pnpm --filter openhuman-app format:check — N/A (no frontend change)
  • pnpm typecheck — N/A (no frontend change)
  • Focused tests: cargo test -p openhuman --lib learning::schemas
  • Rust fmt/check (if changed): cargo fmt
  • Tauri fmt/check (if changed): N/A (core-only change)

Behavior Changes

  • Intended behavior change: facet RPC output gains two already-persisted provenance fields.
  • User-visible effect: provenance (evidence_refs, cue_families) is now retrievable via the learning facet controllers.

Parity Contract

  • Legacy behavior preserved: all existing keys unchanged; opaque-JSON output schema contract unaffected; no store/query change.
  • Guard/fallback/dispatch parity checks: None/empty provenance serializes to null/[] (verified by the test's empty-facet case).

@mysma-9403
mysma-9403 requested a review from a team July 18, 2026 13:11
@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The learning facet JSON serializer now includes cue_families and evidence_refs. Unit tests cover populated and absent provenance values, and the RPC documentation describes the expanded response.

Changes

Facet provenance output

Layer / File(s) Summary
Serialize and validate facet provenance
src/openhuman/learning/schemas.rs, src/openhuman/learning/README.md
facet_to_json now emits cue_families and evidence_refs; tests verify populated, empty, and missing values, while documentation describes the fields.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: senamakel

Poem

I’m a bunny with fields in my nest,
Provenance now travels with every request.
Cue families hop, evidence refs gleam,
JSON reflects the facet’s full dream.

(\_/)
( •_•)
/ >🥕
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: exposing facet provenance fields in learning RPC output.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@mysma-9403

Copy link
Copy Markdown
Contributor Author

CI note — the red checks are not from this change

Rust Core Coverage is red on a pre-existing flaky test, not this PR. The failing test is learning::startup::tests::learning_subscriber_fires_with_no_channel_configured (the email-signature subscriber path in startup.rs). This PR only adds two keys to facet_to_json in schemas.rs plus a serializer unit test — it does not touch startup.rs, the subscriber, or the process-global candidate::global() buffer that test asserts on.

Verified against the PR base d13a559b9 (i.e. without this change): the full openhuman::learning suite — including that test — passes 5/5 local runs. The failure is timing/parallelism-dependent (the test asserts on a shared, process-global candidate buffer); it flaked once under CI parallelism. So it is not a regression introduced here.

Rust Feature-Gate Smoke (gates off) is also unrelated: it fails identically on every fresh-main-based PR with ::error::Gated-test file set changed. Update the EXPECTED allowlist … (see #5022) — a stale allowlist on main, not touched by this change. PR Submission Checklist was my miss (unchecked N/A boxes) and is now fixed in the description.

Happy to open a small follow-up to de-flake that startup test (give it its own isolated candidate buffer) if a maintainer wants it.

…) in RPC

facet_to_json is the single serializer feeding every facet-returning learning
controller (list_facets, get_facet, and the echoed facet on update/pin/unpin/
forget). ProfileFacet persists and row_to_facet hydrates two provenance
columns — evidence_refs (Vec<EvidenceRef>, from evidence_refs_json) and
cue_families (Option<HashMap<String,u32>>, from cue_families_json) — but the
serializer emitted only 10 keys and silently dropped both, so no RPC consumer
could see the evidence behind a facet.

Add the two fields to the json! macro. Both types already derive Serialize
(EvidenceRef is a #[serde(tag = "type")] enum), None/empty serialize to
null/[]. The controller output schemas declare facet/facets as opaque
TypeSchema::Json, so the schema contract is unchanged; the addition is purely
additive metadata. No store or query change.

Scope: evidence_refs are (class, key)-scoped, not value-scoped (the stability
detector merges every candidate's evidence for a key before persisting the one
value). This only stops the serializer dropping already-persisted state.

Test: facet_to_json_includes_cue_families_and_evidence_refs asserts populated
provenance round-trips and empty/None serializes to []/null. README get_facet
row notes the provenance fields.
@senamakel
senamakel force-pushed the feat/learning-facet-provenance-fields branch from e4f6889 to 5df19d9 Compare July 23, 2026 13:34
@greptile-apps

greptile-apps Bot commented Jul 23, 2026

Copy link
Copy Markdown

Greptile Summary

This PR stops the facet_to_json serializer from silently dropping two provenance fields (evidence_refs and cue_families) that the learning store already persists and row_to_facet already hydrates. The fix is two lines in one function; because all six facet-returning RPC controllers share that serializer, they all gain the fields at once without any store or query changes.

  • Adds "cue_families": f.cue_families and "evidence_refs": f.evidence_refs to the serde_json::json! block in facet_to_json, leveraging the existing Serialize impls on Option<HashMap<String,u32>> and Vec<EvidenceRef>.
  • Adds a unit test covering both the populated case (asserts correct field values round-trip) and the empty/None case (asserts []/null are present, not dropped).
  • Updates README.md to document the provenance payload, though only the get_facet row was touched — the same note applies to list_facets and the four mutating controllers.

Confidence Score: 4/5

Safe to merge — the change is purely additive, touches only the serialization boundary, and is backed by a targeted unit test.

The core fix is correct and the change is purely additive at the serialization boundary, backed by a targeted unit test. Two documentation gaps remain — the list_facets schema comment and the README table — but neither affects runtime behavior.

The list_facets schema comment in schemas.rs and the README controller table both need the provenance fields mentioned for list_facets and the other mutating controllers.

Important Files Changed

Filename Overview
src/openhuman/learning/schemas.rs Adds cue_families and evidence_refs to facet_to_json; both fields are correctly typed and tested. The list_facets schema comment's explicit field enumeration is now stale.
src/openhuman/learning/README.md Updates the get_facet row to mention provenance fields, but omits the same update for list_facets and the five other facet-returning controllers that are equally affected.

Sequence Diagram

sequenceDiagram
    participant Client
    participant Controller as learning.* controller
    participant Cache as FacetCache
    participant Store as memory_store::profile
    participant Ser as facet_to_json

    Client->>Controller: RPC call (list_facets / get_facet / update / pin / unpin / forget)
    Controller->>Cache: list_all() / get() / upsert()
    Cache->>Store: SQL query (row_to_facet hydrates evidence_refs, cue_families)
    Store-->>Cache: ProfileFacet (with evidence_refs, cue_families populated)
    Cache-->>Controller: ProfileFacet
    Controller->>Ser: "facet_to_json(&facet)"
    Note over Ser: Previously dropped evidence_refs + cue_families
    Note over Ser: Now includes both fields
    Ser-->>Controller: JSON Value (+ evidence_refs, cue_families)
    Controller-->>Client: RPC response with provenance
Loading

Comments Outside Diff (1)

  1. src/openhuman/learning/schemas.rs, line 243-244 (link)

    P2 The list_facets schema comment explicitly enumerates the fields of each facet object ("key, value, state, user_state, stability") but omits the two newly surfaced provenance fields. Any tooling or client that reads this comment to understand the contract will have a misleading view of the output shape.

Reviews (1): Last reviewed commit: "feat(learning): surface facet provenance..." | Re-trigger Greptile

Comment on lines 64 to +65
| `learning.list_facets` | List Active + Provisional facets, optional `class` filter. |
| `learning.get_facet` | Fetch one facet by `class` + `key` suffix. |
| `learning.get_facet` | Fetch one facet by `class` + `key` suffix. Each returned facet carries its provenance (`evidence_refs`, `cue_families`) alongside the value/state fields. |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The README table was updated only for get_facet, but all six facet-returning controllers (list_facets, get_facet, update_facet, pin_facet, unpin_facet, forget_facet) now carry provenance fields since they all go through facet_to_json. A reader consulting the table for list_facets would not know the provenance fields are present.

Suggested change
| `learning.list_facets` | List Active + Provisional facets, optional `class` filter. |
| `learning.get_facet` | Fetch one facet by `class` + `key` suffix. |
| `learning.get_facet` | Fetch one facet by `class` + `key` suffix. Each returned facet carries its provenance (`evidence_refs`, `cue_families`) alongside the value/state fields. |
| `learning.list_facets` | List Active + Provisional facets, optional `class` filter. Each returned facet carries its provenance (`evidence_refs`, `cue_families`) alongside the value/state fields. |
| `learning.get_facet` | Fetch one facet by `class` + `key` suffix. Each returned facet carries its provenance (`evidence_refs`, `cue_families`) alongside the value/state fields. |

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@senamakel
senamakel merged commit 83d1dc3 into tinyhumansai:main Jul 23, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants