Skip to content

feat: expose external_id in EntityResponse and link resolver#569

Merged
phernandez merged 2 commits intomainfrom
spec-27-external-id-lookup
Feb 16, 2026
Merged

feat: expose external_id in EntityResponse and link resolver#569
phernandez merged 2 commits intomainfrom
spec-27-external-id-lookup

Conversation

@groksrc
Copy link
Member

@groksrc groksrc commented Feb 15, 2026

Add external_id to V1 EntityResponse schema so consumers can reference notes by their stable UUID. Add UUID detection to link resolver so GET /{project}/knowledge/entities/{external_id} and GET /{project}/resource/{external_id} work out of the box.

Add external_id to V1 EntityResponse schema so consumers can reference
notes by their stable UUID. Add UUID detection to link resolver so
GET /{project}/knowledge/entities/{external_id} and
GET /{project}/resource/{external_id} work out of the box.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Drew Cain <groksrc@gmail.com>
@github-actions

This comment was marked as outdated.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2f7359631e

ℹ️ 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".

@groksrc groksrc requested a review from phernandez February 15, 2026 22:26
Address PR #569 feedback:
- Canonicalize parsed UUID to lowercase-hyphen form before lookup so
  uppercase or unhyphenated UUIDs also resolve correctly
- Add 4 tests for UUID resolution: valid ID, uppercase, nonexistent, fallthrough

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Contributor

github-actions bot commented Feb 15, 2026

Claude finished @groksrc's task in 1m 17s —— View job


Updated Review: PR #569

Todo List

  • Read changed files and understand new implementation
  • Check if test coverage was added (previous blocking issue)
  • Review Codex feedback about UUID canonicalization
  • Verify against Basic Memory team checklist
  • Post comprehensive updated review

Review Summary

Excellent work! All blocking issues from my previous review have been addressed. The implementation is clean, follows Basic Memory conventions, and now includes comprehensive test coverage for the UUID resolution functionality.

Changes Since Last Review

  1. Test coverage added ✅ - Four new tests in test_link_resolver.py:868-902:

    • test_resolve_link_by_external_id - Valid UUID resolution
    • test_resolve_link_by_external_id_uppercase - UUID canonicalization (uppercase)
    • test_resolve_link_by_external_id_nonexistent - Non-existent UUID handling
    • test_resolve_link_non_uuid_falls_through - Fallback for non-UUID strings
  2. UUID canonicalization implemented ✅ - The Codex review concern has already been addressed:

    • Line 72 in link_resolver.py: canonical_id = str(uuid_mod.UUID(clean_text))
    • This canonicalizes all valid UUID formats (uppercase, unhyphenated, etc.) to lowercase-hyphen form before lookup
    • Properly documented in comments (lines 69-70)

Code Quality & Standards ✅

  • Follows Basic Memory's coding conventions

    • Excellent literate programming comments explaining UUID canonicalization (lines 67-76 in link_resolver.py)
    • Proper async patterns
  • Python 3.12+ type annotations

    • Proper imports and type hints used throughout
  • 100-character line length limit

    • All lines comply with the limit
  • No commented-out code blocks

    • Clean implementation

Testing & Documentation ✅

  • Unit tests for new functionality

    • Comprehensive test coverage for UUID resolution logic
    • Tests cover all edge cases:
      • Valid UUID lookup via get_by_external_id()
      • UUID canonicalization (uppercase, different formats)
      • Non-existent UUID returns None
      • Invalid UUID strings fall through to normal resolution
  • 100% test coverage maintained

    • All new code paths are covered
    • Previously BLOCKING issue is now resolved
  • Documentation

    • Clear PR description
    • Inline code comments explain the "why" (literate programming style)

Basic Memory Architecture ✅

  • Database changes include Alembic migrations

    • Migration file exists for adding external_id
  • Preserves local-first architecture

    • Changes are additive, non-breaking
  • Knowledge graph operations maintain consistency

    • UUID resolution is read-only, doesn't affect data integrity
  • Schema changes properly implemented

    • EntityResponse.external_id added as optional field (line 198 in response.py)
    • Follows existing patterns

Security & Performance ✅

  • Input validation

    • UUID validation via try/except ValueError pattern is appropriate
    • Canonicalization prevents case/format variations from causing issues
  • Proper error handling

    • Graceful fallback on ValueError (line 77-78)
    • Debug logging when entity found (line 75)
  • Performance considerations

    • UUID check happens early (line 69) before more expensive operations
    • Canonical form ensures consistent database lookups

Compatibility ✅

  • Cross-platform compatibility

    • UUID handling is platform-agnostic
  • No emojis or unicode in logs

    • Clean code

Recommendation

✅ Ready to merge!

All previous blocking issues have been resolved:

  • ✅ Test coverage is now at 100%
  • ✅ UUID canonicalization is properly implemented
  • ✅ All edge cases are tested
  • ✅ Code follows Basic Memory conventions

The implementation is solid and addresses both the original PR goals (exposing external_id in EntityResponse and link resolver) and the Codex feedback about UUID canonicalization.


@phernandez phernandez merged commit 6afe4fd into main Feb 16, 2026
29 checks passed
@phernandez phernandez deleted the spec-27-external-id-lookup branch February 16, 2026 02:33
@phernandez phernandez added this to the v0.19.0 milestone Feb 25, 2026
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