feat: Add project-prefixed permalinks and memory URL routing #544
Conversation
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
|
Claude finished @phernandez's task in 2m 47s —— View job Code Review CompleteReviewing this PR against the Basic Memory team checklist: SummaryThis PR implements project-prefixed permalinks and cross-project memory URL routing. The implementation is comprehensive and well-structured, with extensive test coverage. I've identified several areas that require attention before merging. Code Quality & Standards✅ Strengths
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97cc0d9b4f
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Trigger: local resolution failed and identifier looks like project/path | ||
| # Why: allow explicit project path references without namespace syntax | ||
| # Outcome: attempt resolution in the referenced project if it exists | ||
| project_prefix, remainder = self._split_project_prefix(clean_text) | ||
| if not project_prefix: |
There was a problem hiding this comment.
This new fallback treats any project/path-looking identifier as a cross‑project lookup when local resolution fails. EntityService.create_or_update_entity calls resolve_link on schema.file_path with strict=True, so a file like other-project/foo.md in project A will resolve into project B if a project slug matches other-project and B has a foo permalink. That causes update flows to target another project’s entity (repository updates aren’t project-scoped by id), which is a correctness regression. Consider restricting this fallback to explicit project:: links or guarding create/update callers against cross-project results.
Useful? React with 👍 / 👎.
|
Claude finished @phernandez's task in 2m 44s —— View job Code Review CompleteI've thoroughly reviewed this PR against the Basic Memory team checklist. Here are my findings: SummaryThis PR implements project-prefixed permalinks and cross-project memory URL routing. The implementation is well-structured with comprehensive test coverage and excellent literate programming style. However, I've identified one critical blocker regarding exception handling, plus several issues that should be addressed. Critical Findings🔴 BLOCKER - Exception Handler OrderingIssue: The PR changes Impact: The FastAPI exception handlers in
FastAPI processes exception handlers from most specific to least specific, BUT this depends on Python's MRO. With the new inheritance:
Problem: The exception wrapping at entity_service.py:616 converts Required Action:
Code Reference: # services/exceptions.py:19-22
class EntityAlreadyExistsError(EntityCreationError): # ← NEW HIERARCHY
"""Raised when an entity file already exists"""
pass
# entity_service.py:612-616
try:
return await self.repository.upsert_entity(model)
except Exception as e:
logger.error(f"Failed to upsert entity for {file_path}: {e}")
raise EntityCreationError(f"Failed to create entity: {str(e)}") from e # ← WRAPS ALL EXCEPTIONSSecurity & Correctness Issues
|
phernandez
changed the title
1 participant
Summary
Testing