feat(knowledge): restore client relationship graph actions#8182
Conversation
Restore the client/contact/interaction knowledge graph schema and client_network/interaction_log tool actions from the reverted zeroclaw-labs#4507 work on the current split-crate implementation. Co-authored-by: Nim G <theredspoon@users.noreply.github.com>
| )?; | ||
|
|
||
| let mut results = Vec::new(); | ||
| let mut rows = stmt.query(params![node_type.as_str(), limit as i64])?; |
There was a problem hiding this comment.
This is the only new query method that keeps limit as i64 instead of the checked sql_limit(limit)? you introduced for every other new query in this PR. An out-of-range usize wraps silently here rather than returning the clear error the rest of the path produces. Make it let limit = sql_limit(limit)?; for consistency.
|
Draft, so direction only, no verdict. I read the full diff: the enum macro consolidation, the new graph query methods, the client/contact/interaction node and relation types, the tool actions, and the locale additions. Direction notes for when you take this out of draft:
Flag me when it is ready for a verdict. |
|
@singlerider thanks, this was helpful. I tightened the checked-limit point in I am going to leave the action-dispatch enum as a follow-up candidate for now. The string dispatch predates this PR, and the current action/schema list is covered by tests, so I do not want to expand this recovery slice further just to reshape that internal routing. I will flag you again after the broad validation / ready pass. |
singlerider
left a comment
There was a problem hiding this comment.
Re-reviewed at head 88faab6 (was 24416de on my prior COMMENT). The only change since is the limit fix I asked for.
✅ Resolved — checked SQL limits applied consistently
My one blocker was find_inbound_by_relation binding limit as i64 while every other new query routed through sql_limit(limit)?. Fixed: each of the six new query methods now shadows let limit = sql_limit(limit)?; at the function top and binds the checked value, so an out-of-range usize returns the clear "exceeds SQLite range" error instead of wrapping silently. The remaining as i64 is on subgraph depth, which is clamped to MAX_SUBGRAPH_DEPTH (100) and rejects 0, so that cast cannot overflow.
🟢 What looks good — schema additions are additive and the surfaces stay in lockstep
New Client/Contact/Interaction nodes and the three relations keep existing graph strings untouched, so the change is backward compatible. The Postgres converter and all five localized tool-knowledge descriptions move with the schema, so feature-gated and prompt-visible surfaces do not drift. Supersede attribution is honest: #4507 carried forward with a Co-authored-by trailer for @theredspoon.
🟢 What looks good — bounded, validated, deterministic reads
User-supplied limits are bounded at the tool layer and re-checked in storage, client actions require client_id to resolve to a real Client node, and neighbor ordering is deterministic before formatting. The validation battery is thorough for a risk:high change (full workspace clippy + nextest green, plus the Postgres feature path).
Verdict: APPROVED.
Summary
masterClient,Contact, andInteractionnode types plusmanages_client,contact_of, andinteracted_withrelations.graph_neighbors,client_network, andinteraction_logknowledge tool actions so agents can inspect generic graph links, a client account's contacts and relationship owner, and recent interactions from the graph.tool-knowledgedescriptions so feature-gated and prompt-visible surfaces do not drift from the new schema.zeroclaw-memorygraph enum parsing and query helpers,zeroclaw-toolsknowledge tool schema/actions,memory-postgresconversion helpers, and localized runtime tool descriptions.enhancement,risk: high,size: L,tool,memory,memory: postgresValidation Evidence
graph_neighborsresults, bounded user-supplied limits,client_network,interaction_log, invalid client IDs, malformed manager edges, localized tool description freshness, and Postgres enum conversion behavior.risk: highPR.Security & Privacy Impact
Yes. This adds knowledge tool actions that read existing graph relationships for a supplied node or client ID. It does not add new filesystem access, network access, or a bypass around the existing tool permission path.NoNoNoYes, describe the risk and mitigation: The new actions can expose knowledge graph entries to an agent that is already allowed to call theknowledgetool. The implementation validates requested nodes, requiresclient_idto resolve to aClientnode for client-specific actions, bounds result counts, and keeps tests on neutral placeholder data.Compatibility
YesNoNoorYesto either: Existing users do not need migration steps. Existing graph node and relation strings keep their current spelling, and the new variants/actions are additive.Rollback
graph_neighbors,client_network, orinteraction_logfail or return unexpected graph data. Search logs forknowledge_tool,unknown node type,unknown relation, orclient action missing client_id.Supersede Attribution
client_network/interaction_logknowledge tool actions were restored on the current split-crate implementation.Co-authored-bytrailers added in commit messages for incorporated contributors?YesNo, why: Not applicable.