Skip to content

fix(cli): use resolved project path in doctor command#667

Open
phernandez wants to merge 1 commit intomainfrom
fix/665-doctor-project-root
Open

fix(cli): use resolved project path in doctor command#667
phernandez wants to merge 1 commit intomainfrom
fix/665-doctor-project-root

Conversation

@phernandez
Copy link
Member

Summary

  • When project_root is configured (e.g., Docker), bm doctor would falsely fail because it checked for files under the original temp_path instead of the resolved path under project_root
  • Now uses status.new_project.path (the actual resolved path) for all file existence checks after project creation

Fixes #665

Test plan

  • just fast-check passes (lint, typecheck, impacted tests)
  • just doctor passes locally (without project_root set)
  • Verify with project_root configured that doctor no longer false-fails

🤖 Generated with Claude Code

When project_root is configured (e.g., Docker), the project service
creates the project under project_root/<sanitized-name> instead of
the requested temp_path. Doctor was checking for files under the
original temp_path, causing false failures.

Now uses status.new_project.path (the actual resolved path) for all
file checks after project creation.

Fixes #665

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: phernandez <paul@basicmachines.co>
@github-actions
Copy link
Contributor

github-actions bot commented Mar 14, 2026

Claude finished @phernandez's task in 1m 44s —— View job


PR Review

  • Read changed files and gather context
  • Review code quality and standards
  • Check testing coverage
  • Evaluate architecture compliance
  • Post final review

Summary

This is a correct and minimal fix. When project_root is configured (e.g., Docker deployments), the server's project_service.py ignores the user-supplied path and creates the project under {project_root}/{sanitized_name}. The doctor command was using temp_path for all file checks, which would fail since the actual files live under project_root. The fix uses status.new_project.path (the authoritative server-resolved path) instead.

Code Review Checklist

Code Quality & Standards

  • Follows Basic Memory's coding conventions in CLAUDE.md
  • Python 3.12+ type annotations and async patterns
  • 100-character line length maintained
  • No commented-out code blocks — comment explains the "why" per literate programming style
  • Minimal diff — only 3 lines changed in the core logic, well-scoped
  • N/A — No SQLAlchemy, FastAPI, or Typer changes in this PR

Testing & Documentation

  • No test coverage for the fixed bug — There are zero tests for the doctor command in the entire codebase (tests/ and test-int/). The CLAUDE.md requires 100% coverage. Given that the doctor command is inherently an end-to-end integration scenario (real filesystem + API + sync), a basic unit test mocking project_client.create_project to return a different path would at least cover the project_path = Path(status.new_project.path) substitution. Consider as a follow-up.
  • Style-only changes in the other 3 files (test_string_params_integration.py, test_tool_edit_note.py, test_coerce.py) — blank line removals, no logic changes

Basic Memory Architecture

  • Correct use of status.new_project.path — consistent with how project_router.py:134 and project_management.py:370 use the same field
  • No knowledge graph / markdown file integrity concerns
  • Local-first principles preserved

Security & Performance

  • No secrets, credentials, or sensitive data
  • No new input validation surface area
  • No performance concerns — read of a string field, one Path() construction

Compatibility

  • Uses Path() which is cross-platform (Windows-safe)
  • No emoji or unicode in console/log output

One Potential Follow-up: Cleanup Under project_root

doctor.py:130 — The finally block calls delete_project(project_id) without delete_notes=True:

await project_client.delete_project(project_id)

Before this fix, in project_root mode the command would fail before creating any files — so no leftover files existed. Now that the fix works correctly, doctor will successfully write files to {project_root}/doctor-{uuid}/, but the cleanup only removes the DB/config entry. The TemporaryDirectory context manager only cleans up temp_path (which is now empty), leaving test artifacts under project_root.

In a Docker deployment, this means each bm doctor run under project_root leaves behind a doctor-{uuid}/ directory. Not a blocker for merging, but worth tracking.

Suggested follow-up →


Overall: LGTM — The fix is correct, well-commented, and appropriately scoped. The cleanup concern above is a follow-up, not a blocker.

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.

bm doctor false failure when project_root is configured

1 participant