fix(runtime,kg): namespace-agnostic by-ID CRUD short-prefix resolution (#391 §3, #395 lane)#393
Merged
Merged
Conversation
…ered) targets under Rev 6 (#391) brain.auto_feedback/brain.feedback have errored ("no record matches id prefix" / "target_id ... not found in namespace ...") for every episodic memory written since 2026-06-19. Root cause: two independent id-resolution seams never picked up the ADR-007 Rev 6 visible-set model. Two distinct policies are fixed here, kept deliberately separate: 1. Feedback (legs B/C) -- visible-set-aware resolution. `resolve_auto_feedback_target` (hex-prefix, leg B) and `handle_feedback` (exact UUID, leg C) now resolve against the token's visible-namespace set, mirroring how memory.recall already reads across {local} union {actor.id} union {actor.visible_namespaces}. New `resolve_prefix_visible` sits alongside the pre-existing `resolve` (exact-UUID visible-set variant), completing the strict/visible pair for prefix resolution the same way `resolve_primary`/`resolve` already pairs for exact UUIDs. 2. By-ID CRUD (get/update/delete/merge) -- fully unfiltered resolution. Empirical finding: full-UUID by-ID lookups (`resolve_by_id`, `get_note`, `update_note`, `merge_*`) were already namespace-agnostic, per ADR-007 Rev 6's own rule that by-ID ops carry no visibility boundary (the Gate is the authz seam, not storage-layer filtering). Only the hex-prefix expansion step (`resolve_uuid_async`, common.rs) was still primary-namespace-only, silently narrowing prefix lookups to a boundary the full-UUID path doesn't have. New `resolve_prefix_unfiltered`/`resolve_prefix_unfiltered_including_deleted` (operations.rs) and `resolve_uuid_unfiltered`/ `resolve_uuid_unfiltered_including_deleted` (kg common.rs) close that gap; wired into get.rs, update.rs (update + soft/hard delete), and merge.rs only. `resolve_prefix_inner` takes `namespaces: Option<&[String]>` so a single implementation serves all three resolution policies (`Some([primary])` for strict, `Some(visible_set)` for feedback, `None` for by-ID CRUD) via thin wrappers, rather than duplicating the prefix-matching SQL a third time. Deliberately untouched: - `resolve_primary` and strict `resolve_prefix` (via `resolve_uuid_async`) for gtd `depends_on` validation and link/edge endpoint validation -- their primary-namespace-only strictness is a tested contract for those callers, not a bug. - `resolve_uuid_async` call sites in link.rs, create.rs, list.rs, graph.rs -- same root cause family, deliberately out of scope; list/search keep their namespace filter by design. - `merge_entity`'s namespace-ownership check (curation.rs) -- a separate, pre-existing mutation-safety mechanism downstream of id resolution. Merge-by-prefix now resolves cross-namespace (proven by the changed error: resolution success -> ownership-check rejection, not resolution failure), but the ownership check itself still legitimately blocks a cross-namespace merge. Not in scope for #391. - Leg A (profile-registry per-session staleness) -- tracked separately, unrelated to id resolution. Regression coverage: - khive-pack-brain: `resolve_auto_feedback_target`/`handle_feedback` visible-set behavior -- 3 positive shapes (primary, extra-visible, hex-prefix-in-extra-visible) + 1 negative (third-party, non-visible namespace) (src/tests.rs). dispatch_hook.rs's stale `brain_feedback_rejects_visible_only_target_id` test (asserting the bug's own behavior, from PR #127) is renamed `brain_feedback_accepts_visible_only_target_id` and inverted to assert the corrected behavior. - khive-pack-kg integration.rs: 8 new tests for by-ID CRUD's unfiltered policy -- get/update/soft-delete/hard-delete by prefix cross-namespace now succeed; full-UUID cross-namespace guard retained; merge by prefix cross-namespace resolves then correctly hits the separate ownership check (not a resolution error); merge by prefix same-namespace succeeds; prefix matching nothing still raises NotFound. Gates (all from crates/, rc 0): cargo fmt --all -- --check cargo clippy --workspace --all-targets -- -D warnings cargo test --workspace --locked RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --workspace cargo test -p khive-pack-gtd --test dependencies --locked (no-regression) cargo test -p khive-runtime --test integration --locked (no-regression) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… (codex r1) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ltered by-ID contract test_read_isolation_between_namespaces asserted the pre-#391 policy: an 8-char prefix get from a foreign namespace must return not-found because prefix expansion was namespace-scoped. This PR deliberately corrects that contract — by-ID prefix resolution is now unfiltered, matching the full-UUID by-ID path (ADR-007 Rev 6 Rule 2). The assertion now expects the cross-namespace prefix to resolve and return the entity. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eedback to #498 #498 (merged 2026-07-03) fixed the #391 feedback outage with a fully namespace-agnostic policy (resolve_by_id, ADR-007 Rule 2), superseding this branch's earlier visible-set-aware feedback approach. Per Ocean's ruling (2026-07-03), defer the feedback leg entirely to #498 and land only this branch's still-novel by-ID CRUD short-prefix unfiltered leg — which #498 explicitly left out of scope (short-prefix "tracked separately under #395"). Resolution: - brain handlers.rs / tests.rs / dispatch_hook.rs -> reset to main (#498 wins). - operations.rs -> keep the resolve_prefix_inner(namespaces: Option<&[String]>) refactor + resolve_prefix_unfiltered*; DROP the now-orphaned resolve_prefix_visible (belonged to the dropped feedback leg). - kg get/update/merge/common.rs + integration.rs + contract py -> keep (by-ID leg). Net effect vs main: by-ID CRUD prefix resolution (get/update/delete/merge) is now namespace-agnostic, matching its already-unfiltered full-UUID path. No feedback-policy change. Consistent with #498's ADR-007 Rev 6 by-ID contract. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ohdearquant
changed the title
4 tasks
ohdearquant
added a commit
that referenced
this pull request
Jul 3, 2026
…ty, namespace-agnostic event get, graph direction defaults (#517) * fix(pack-kg): reject non-atomic compound proposals (#423) A Compound proposal with a successful AddEntity step followed by a later step that fails (e.g. AddEdge with an invalid endpoint) used to leave the first step's write committed while the proposal reported applied_step_count=0 and reverted to `approved`. There is no transaction primitive across khive-runtime/khive-db entity, note, edge, merge, and event writes to make Compound apply atomic within this crate, so contain the bug instead: reject multi-step Compound proposals both at propose time and (for any legacy queued proposal) at apply time, before any step runs. Cross-crate escalation: a real fix needs a caller-controlled transaction boundary spanning khive-runtime/khive-db mutation APIs (entity/note/edge/event stores each currently commit their own transaction per call). Out of scope for khive-pack-kg alone. Adds a regression test asserting a compound with a successful AddEntity followed by an invalid AddEdge leaves no new entity and reports zero applied steps. Updates the two flat/nested Compound budget tests whose old expectations (multi-step compounds applying, or failing specifically with WriteBudgetExceeded) are superseded by this containment fix. * fix(pack-kg): validate proposed AddEntity like create (#424) Proposal apply for AddEntity validated the entity kind against the base khive_types::EntityKind::ALL taxonomy, not the pack-local vocab (which adds `resource`), so a valid-looking AddEntity{kind="resource"} proposal would be accepted at propose time and then fail at apply. It also passed draft.name straight to runtime.create_entity with no whitespace check, so AddEntity{name=" "} could create a blank-name entity that the normal `create` handler would reject. apply_add_entity now resolves the kind through the same canonical_entity_kind() helper the normal create path uses (pack vocab + registry-registered kinds, not just the base taxonomy), and rejects whitespace-only names before the runtime write. Adds regression tests: AddEntity(kind="resource") now succeeds and resolves to kind="resource"; AddEntity(name=" ") fails with zero net entity writes and a ProposalApplied{Failed} payload naming the empty-name guard. * fix(pack-kg): make event by-id get namespace agnostic (#425) PR #393 made entity/note/edge by-ID `get` namespace-agnostic (ADR-007 Rev 6), but the event-UUID resolver path was a separate code path that #393 did not touch: `get(id=<event_uuid>)` still fetched through `runtime.events(token)?.get_event(id)`, which is namespace-scoped, so a caller in a different namespace than the event got NotFound. Adds a private pack-local `get_event_unfiltered_by_id` helper that selects the event row by `id` with no namespace predicate, reusing the existing `common::parse_event_kind` / `parse_event_outcome` / `parse_event_substrate` helpers to reconstruct the `Event`. Only the by-ID `get` path is unfiltered; event `list`/`query` keep their namespace scoping unchanged. Adds a regression test: an event created in one namespace is fetched via `get(id=<event_uuid>, namespace="ns-caller")` from a different namespace and must succeed with the event's original namespace preserved in the response. * fix(pack-kg): honor graph direction defaults (#445, #480) neighbors' own live help=true schema advertises direction default "both", but parse_direction(None) returned Direction::Out — every caller omitting direction (the common case) silently got outgoing-only instead of the advertised both-directions default (#445). The same catch-all also silently coerced any unrecognized direction string (e.g. a plausible typo like "inbound") to Direction::Out instead of rejecting it (#480). Both bugs share the same root cause and fix, so one commit resolves both issue numbers. parse_direction now returns Result<Direction, RuntimeError>: omitted direction resolves to Direction::Both (matching the advertised contract), and unrecognized non-empty strings are rejected with an error listing the valid values. Both neighbors and traverse call sites propagate the new Result via `?`. Adds regression tests on an incoming-only graph (A --extends--> B, no outgoing edges from B): neighbors/traverse with direction omitted now surface the incoming edge from A, and direction="inbound" is rejected with a descriptive error instead of silently returning outgoing-only results.
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.
Scope (revised 2026-07-03 — feedback leg deferred to #498)
This PR now lands only the by-ID CRUD short-prefix resolution fix. The
feedback-resolution leg it originally carried was superseded by #498
(merged 2026-07-03), which fixed the #391 feedback outage with a fully
namespace-agnostic policy (
resolve_by_id, ADR-007 Rule 2). Per Ocean'sruling, feedback policy is deferred entirely to #498; this branch keeps only
its still-novel by-ID leg, which #498 explicitly left out of scope
("short-prefix stays namespace-scoped… tracked separately under #395").
What this fixes
The full-UUID by-ID path (
get/update/delete/merge) is alreadynamespace-agnostic (ADR-007 Rev 6: by-ID ops carry no visibility boundary; the
Gate is the authz seam). But the short-prefix expansion step
(
resolve_prefixviaresolve_uuid_async) was still primary-namespace-only,silently narrowing prefix lookups to a boundary the full-UUID path never had. A
caller in a different namespace than a record got
NotFoundfor a prefix thatwould have resolved fine as a full UUID.
Implementation
operations.rs:resolve_prefix_innernow takesnamespaces: Option<&[String]>—Some([primary])for the existing strictresolve_prefix/resolve_prefix_including_deleted(unchanged behavior),Nonefor the newresolve_prefix_unfiltered/resolve_prefix_unfiltered_including_deleted.khive-pack-kg/src/handlers/common.rs:resolve_uuid_unfiltered/resolve_uuid_unfiltered_including_deletedwrappers, wired intoget.rs,update.rs(update + soft/hard delete), andmerge.rsonly.*_by_prefix_cross_namespace_*integration tests + invertedcontract assertion in
test_namespace_isolation.py.Deliberately untouched
resolve_prefix(viaresolve_uuid_async) — used by gtddepends_onvalidation, link/edge endpoint validation, create/list/graph.Their primary-namespace-only strictness is a tested contract, not a bug
(
resolve_prefix_invisible_across_namespacesstays green).merge_entity's namespace-ownership check (curation.rs) — a separatemutation-safety mechanism downstream of id resolution. Merge-by-prefix now
resolves cross-namespace, then the ownership check legitimately rejects the
cross-namespace merge (no longer a resolution-layer NotFound).
Verification
cargo check+clippy -D warnings+fmt --checkclean; 8/8 prefixintegration tests pass;
resolve_prefixunit tests green; #498'sbrain_feedback_accepts_foreign_namespace_target_idstill passes (brainidentical to main).