Skip to content

fix(api): accept qualified project resolver hints#795

Merged
phernandez merged 1 commit intomainfrom
codex/793-qualified-project-resolve
May 6, 2026
Merged

fix(api): accept qualified project resolver hints#795
phernandez merged 1 commit intomainfrom
codex/793-qualified-project-resolve

Conversation

@phernandez
Copy link
Copy Markdown
Member

Summary

Test Plan

  • uv run pytest tests/api/v2/test_project_router.py::test_resolve_project_by_workspace_qualified_permalink
  • uv run pytest tests/api/v2/test_project_router.py
  • uv run pytest tests/mcp/test_project_context.py
  • uv run ruff check src/basic_memory/api/v2/routers/project_router.py tests/api/v2/test_project_router.py
  • uv run ruff format --check src/basic_memory/api/v2/routers/project_router.py tests/api/v2/test_project_router.py
  • uv run ty check src/basic_memory/api/v2/routers/project_router.py tests/api/v2/test_project_router.py
  • git diff --check
  • just fast-check (initial cold run reached 2847 passing tests, then failed 11 CLI routing tests because local user config had Logfire send-to-Logfire enabled and prompted for credentials; reran with BASIC_MEMORY_LOGFIRE_ENABLED=false, and the selected 11-test failure slice passed)
  • just doctor

Closes #793.

Signed-off-by: phernandez <paul@basicmachines.co>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 6, 2026

Claude finished @phernandez's task in 2m 6s —— View job


PR Review: fix(api): accept qualified project resolver hints

  • Read changed files and gather context
  • Analyze code quality and standards
  • Check testing and coverage
  • Verify architecture patterns
  • Review security and performance
  • Post final review

Summary: Clean, focused refactor that extracts resolver logic from resolve_project_identifier into three well-structured helpers. The core change is correct and the architecture is solid. A few minor issues are noted below.


Code Quality & Standards

  • Python 3.12+ type annotations — correct use of tuple[str | None, str] union syntax

  • FastAPI conventions followed — endpoint signature unchanged

  • 100-character line length maintained

  • No commented-out code blocks

  • Import orderingProject and ProjectRepository imports are inserted between two basic_memory.schemas import groups (lines 27–29), splitting what was a contiguous schema block. A clean grouping would be:

    from basic_memory.models import Project
    from basic_memory.repository.project_repository import ProjectRepository
    from basic_memory.schemas import SyncReportResponse
    from basic_memory.schemas.project_info import (...)
    from basic_memory.schemas.v2 import ProjectResolveRequest, ProjectResolveResponse
  • ProjectResolveMethod type alias placement — defined at line 41, after router = APIRouter(...). Type aliases are typically grouped near the imports before any module-level objects. Minor but inconsistent. Fix this →


Testing & Coverage

  • New test test_resolve_project_by_workspace_qualified_permalink correctly exercises the workspace/project path
  • # pragma: no cover on the "name" return branch (line 73) — acceptable; this branch existed in the original code and would require injecting a project whose name differs from its permalink slug, which is an awkward fixture to construct
  • Test assertion brittleness (line 336): The new test asserts resolution_method == "permalink" exactly, while the existing test_resolve_project_by_name and test_resolve_project_by_permalink use the more flexible in ["name", "permalink"]. This is acceptable if test_project.name is always permalink-safe, but consider using the same flexible assertion for consistency:
    assert resolved.resolution_method in ["name", "permalink"]
  • No coverage for "name" resolution via qualified path: If test_project.name were something that get_by_permalink missed but get_by_name_case_insensitive caught (e.g., name with spaces), the "name" branch of _resolve_project_identifier_candidate would be exercised through the qualified path. No test covers this sub-case, though it's acceptable given the pragma: no cover rationale.

Basic Memory Architecture

  • Atomic, composable design — helpers are private (_-prefixed), single-responsibility
  • Preserves the resolver response schema and all existing resolution methods
  • Decision-point comments correctly follow CLAUDE.md literate-programming style (Trigger/Why/Outcome) at lines 94–97

Edge Cases

  1. Deep path a/b/c_split_qualified_project_identifier("a/b/c") returns ("a", "b/c"). The b/c segment is then passed to _resolve_project_identifier_candidate, which calls generate_permalink("b/c")"b/c" (slash preserved). This is functionally harmless but the behavior is implicit; a comment noting that only the first segment is treated as workspace would clarify intent.

  2. Whitespace around slash"personal / project-name" (spaces around /) would split to workspace_identifier = "personal " (trailing space) and project_identifier = " project-name" (leading space). The leading space would cause generate_permalink(" project-name") to produce "project-name" (stripping the space), which might accidentally resolve correctly. Stripping workspace_identifier and project_identifier individually would be safer:

    workspace_identifier, project_identifier = cleaned.split("/", 1)
    workspace_identifier = workspace_identifier.strip()
    project_identifier = project_identifier.strip()

    Fix this →


Docstring Gap

The endpoint docstring for resolve_project_identifier lists supported identifier types (external_id, permalink, name) but doesn't mention the new workspace/project qualified form. Since this is a public API endpoint, it would help to document this:

# In the endpoint docstring:
- Qualified workspace/project form (e.g., "personal/my-project") — workspace segment is ignored, project segment is resolved

Fix this →


Checklist Summary

Category Status
Code conventions ✅ (minor import ordering)
Type annotations
FastAPI/SQLAlchemy patterns
100% coverage maintained ✅ (pragma justified)
Tests for new behavior
Architecture / composability
Literate-programming comments
Security / no hardcoded secrets
Input validation
Edge case: whitespace around slash ⚠️ minor
Docstring completeness ⚠️ minor

Overall: The implementation is correct and well-structured. The Trigger/Why/Outcome comment at lines 94–97 is exemplary. The whitespace-around-slash edge case and docstring gap are the only items worth addressing before merge, both minor.

@phernandez phernandez merged commit b08659e into main May 6, 2026
25 checks passed
@phernandez phernandez deleted the codex/793-qualified-project-resolve branch May 6, 2026 15:33
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.

MCP error message hints at qualified project format (workspace/project) that the API doesn't accept

1 participant