fix(nams): handle auto-merged entity creates in both SDKs; fix crewai smoke test - #185
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Andy2003
left a comment
Andy2003
left a comment
There was a problem hiding this comment.
Merged-create handling works; findings below are uncovered edge cases. Verified at head: tests/unit/nams 380 passed, tsc --noEmit clean, vitest run test/unit 108 passed. The crew_id fix is correct (crew_id is a required param).
| _SPEC_GET_ENTITY, | ||
| path_params={"entity_id": _to_str(merged_id)}, | ||
| ) | ||
| except MemoryError: |
There was a problem hiding this comment.
MemoryError is the root of the NAMS error tree, so this also swallows AuthenticationError (401/403), RateLimitError (429) and TransportError (5xx/network). Only 404 maps to a bare MemoryError (transport.py:358).
Effect: a rate-limited or auth-failed canonical read returns the synthesized entity with the requested name — no error, no log.
Needs a NotFoundError in core/exceptions.py, or except MemoryError as exc: + if type(exc) is not MemoryError: raise.
| "id": merged_id, | ||
| "name": name, | ||
| "type": nams_type, | ||
| "confidence": payload.get("confidence"), |
There was a problem hiding this comment.
Reusing the merge score as Entity.confidence:
- It is a name-similarity score, not extraction confidence. The canonical-fetch path returns the entity's own value, so the field means different things per path — a caller filtering
confidence >= 0.95drops a canonical entity whose stored value is 1.0. Entity.confidenceisField(ge=0.0, le=1.0); an out-of-range score makes this branch raise theValidationErrorit exists to prevent.
Drop it (model default 1.0) or move to metadata.
| from datetime import datetime, timezone | ||
|
|
||
| data = snakeize_keys(payload) if isinstance(payload, dict) else {} | ||
| data = {k: v for k, v in data.items() if v is not None} |
There was a problem hiding this comment.
Runs on every entity parse, not just merges, and strips identity fields.
"id": null-> client-side UUID viaMemoryEntry.id'sdefault_factory=uuid4.search_entitiescan then return entities whose id does not exist server-side; a laterget_entity_relationships(entity.id)404s with no clue why."created_at": null-> silentlynow().
Restrict to fields with meaningful defaults (confidence, description, subtype, updated_at).
| ) | ||
| payload = await self._transport.request(_SPEC_ADD_ENTITY, json=body) | ||
| if isinstance(payload, dict) and payload.get("resolution") == "merged": | ||
| payload = await self._resolve_merged_entity(payload, name=name, nams_type=body["type"]) |
There was a problem hiding this comment.
resolution / merged_into / merge confidence are discarded — caller asks for "Alice Smith", gets "Alice" under a different id, no signal.
_normalize_entity already defaults metadata to {}; {"resolution": "merged", "merged_into": ..., "merge_confidence": ...} there is non-breaking.
| try { | ||
| return await this.getEntity(mergedId); | ||
| } catch (error) { | ||
| if (!(error instanceof MemoryError)) throw error; |
There was a problem hiding this comment.
Same over-broad catch as Python: MemoryError is the base class, so AuthenticationError / ValidationError / TransportError all fall through to the synthesized entity.
Caveat on the obvious fix: NotFoundError (errors.ts:44) is exported but never thrown in src/ — rest.ts:622-648 throws TransportError with status for every non-ok response, 404 included. Narrowing to NotFoundError disables the fallback entirely. Use error instanceof TransportError && error.status === 404.
| const mergedId = wire.merged_into ?? wire.id; | ||
| if (mergedId) { | ||
| try { | ||
| return await this.getEntity(mergedId); |
There was a problem hiding this comment.
Missing Python's guard (long_term.py:270, if isinstance(detail, dict) and detail.get("name")): a 200 whose body lacks name returns {name: undefined} — the bug this PR fixes, one hop later.
Sharper: rest.ts:615 (204) and rest.ts:652 (empty body) return undefined, so getEntity -> toEntity(undefined) throws a raw TypeError reading .id. Not a MemoryError, so it escapes the catch below and propagates out of addEntity.
| return toEntity({ | ||
| id: mergedId ?? "", | ||
| name, | ||
| type: entityType, |
There was a problem hiding this comment.
entityType verbatim diverges from every other path, which returns the server's lowercase echo: addEntity("x", "PERSON") gives type: "person" on create and on merged+GET, but "PERSON" here. Python passes the already-mapped body["type"] through the same normalization.
Also createdAt lands as "" (toEntity, line 135) -> new Date(entity.createdAt) is Invalid Date. Python synthesizes datetime.now(timezone.utc).
| updatedAt: w.updated_at ?? undefined, | ||
| confidence: w.confidence ?? undefined, | ||
| sourceStage: w.source_stage ?? undefined, | ||
| relationships: w.relationships?.map(toRelRef) ?? undefined, |
There was a problem hiding this comment.
Normalization stops at toEntity — toRelRef (and toPreference / toFact / toMention) still pass nulls through. The merged-entity GET returns inlined relationships, so nulls leak on exactly the path this PR adds: EntityRelationshipRef.targetName / properties can be null despite being typed | undefined.
2 participants
Fixes the two failing tests in the NAMS integration CI run:
tests/integration/nams/test_tck_platinum.py::test_get_entity_historytests/integration/nams/test_framework_smoke.py::test_crewai_smokeadd_entitybreaks when NAMS auto-merges the create (real client bug, both SDKs)NAMS resolves-before-create: when a new entity name is sufficiently similar to an
existing entity,
POST /v1/entitiesmerges onto it and responds{"id": "...", "resolution": "merged", "merged_into": "...", "confidence": 0.93}with no name/type (verified against the server's entity_create.go). The
platinum test tripped this because the shared sandbox accumulates similarly-named
itest-*-history-target-1 entities across CI runs — the failure was
sandbox-state-dependent, not specific to get_entity_history (the traceback's
add_entity call was the actual failure point).
response as an Entity raised pydantic.ValidationError. add_entity now
detects resolution: "merged", follows up with GET /entities/{merged_into},
and returns the canonical merged-into entity. If that read fails, it
synthesizes a parseable Entity from the request's name/type plus the
returned id and confidence.
failure mode — no runtime validation, so addEntity silently returned an
Entity with undefined name and type. Same fix: follow-up getEntity,
fallback on SDK errors (MemoryError subclasses), non-SDK errors still
propagate.
created and review_pending responses carry full entity fields and are
unaffected (locked in by tests).
test_crewai_smoke constructed the adapter wrong (test bug)
The test called Neo4jCrewMemory(memory_client=nams_client) without the
required crew_id — the docs and tests/integration/test_crewai_integration.py
all pass it. The test now passes crew_id=f"{nams_session}-crew", and the stale
comment claiming the adapter uses an internal crew id is corrected.
Tests
(merged → canonical fetch, merged + 404 → request-field fallback,
review_pending → parses directly).
above plus a non-SDK-error rethrow case.
Verification
clean on the changed files; both integration files collect cleanly (live runs
need sandbox credentials).
npm run build succeeds — the same jobs the TS PR CI runs. The bridge TCK
suite is env-gated and unaffected (bridge responses never carry
resolution: "merged").