Fix sphinx-build discovery and improve rebuild error handling by bburda · Pull Request #1 · bburda/ai_memory_protocol

bburda · 2026-02-08T17:47:19Z

Add find_sphinx_build() to engine.py that searches workspace venv, parent/sibling venvs, and system PATH (fixes FileNotFoundError when sphinx-build is not on PATH but exists in a nearby venv)
Add run_rebuild() to engine.py as shared rebuild logic (DRY)
CLI cmd_rebuild and MCP _do_rebuild now use shared run_rebuild()
MCP memory_add reports add success separately from rebuild failure (previously a rebuild error made it look like the add itself failed)
Remove unused subprocess imports from cli.py and mcp_server.py

- Add find_sphinx_build() to engine.py that searches workspace venv, parent/sibling venvs, and system PATH (fixes FileNotFoundError when sphinx-build is not on PATH but exists in a nearby venv) - Add run_rebuild() to engine.py as shared rebuild logic (DRY) - CLI cmd_rebuild and MCP _do_rebuild now use shared run_rebuild() - MCP memory_add reports add success separately from rebuild failure (previously a rebuild error made it look like the add itself failed) - Remove unused subprocess imports from cli.py and mcp_server.py

Copilot

Pull request overview

Improves reliability of Sphinx rebuilds by centralizing rebuild logic in engine.py, adding more robust sphinx-build discovery, and aligning CLI/MCP behavior so “add” success isn’t masked by rebuild failures.

Changes:

Added find_sphinx_build() and shared run_rebuild() in engine.py.
Updated CLI rebuild and MCP rebuild/add flows to use run_rebuild() and improved messaging separation.
Removed unused subprocess imports and refactored related code paths.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File	Description
src/ai_memory_protocol/engine.py	Adds sphinx-build discovery + shared rebuild runner used by CLI and MCP.
src/ai_memory_protocol/cli.py	Switches `memory rebuild` to shared rebuild logic and simplifies output.
src/ai_memory_protocol/mcp_server.py	Uses shared rebuild logic and separates “add succeeded” from “rebuild failed”.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-08T17:50:07Z

src/ai_memory_protocol/mcp_server.py

-    for n in needs.values():
-        by_type[n.get("type", "?")] = by_type.get(n.get("type", "?"), 0) + 1
-    return f"Rebuilt successfully. {len(needs)} memories: {', '.join(f'{k}={v}' for k, v in sorted(by_type.items()))}"
+    success, message = run_rebuild(workspace)


success is assigned but never used here. With ruff/pyflakes enabled (F841), this will fail lint; use _/_success or return based on it.

Suggested change

success, message = run_rebuild(workspace)

_, message = run_rebuild(workspace)

Copilot · 2026-02-08T17:50:08Z

src/ai_memory_protocol/engine.py

+    for parent in workspace.parents:
+        candidate = parent / ".venv" / "bin" / "sphinx-build"
+        if candidate.exists():
+            return str(candidate)
+        # Also check sibling directories (e.g., ../ros2_medkit/.venv/)
+        if parent.is_dir():
+            for sibling in parent.iterdir():
+                if sibling.is_dir() and sibling != workspace:
+                    candidate = sibling / ".venv" / "bin" / "sphinx-build"
+                    if candidate.exists():
+                        return str(candidate)
+            break  # Only check immediate parent's siblings
+


find_sphinx_build() claims to “walk parent directories”, but the unconditional break after checking the immediate parent’s siblings stops the loop after the first parent. This means venvs in higher ancestors (e.g., grandparent /.venv/) will never be discovered; remove the loop break (or gate sibling scanning to only the first parent) so the parent walk actually continues.

Suggested change

for parent in workspace.parents:

candidate = parent / ".venv" / "bin" / "sphinx-build"

if candidate.exists():

return str(candidate)

# Also check sibling directories (e.g., ../ros2_medkit/.venv/)

if parent.is_dir():

for sibling in parent.iterdir():

if sibling.is_dir() and sibling != workspace:

candidate = sibling / ".venv" / "bin" / "sphinx-build"

if candidate.exists():

return str(candidate)

break # Only check immediate parent's siblings

for i, parent in enumerate(workspace.parents):

candidate = parent / ".venv" / "bin" / "sphinx-build"

if candidate.exists():

return str(candidate)

# Also check sibling directories of the immediate parent

if i == 0 and parent.is_dir():

for sibling in parent.iterdir():

if sibling.is_dir() and sibling != workspace:

candidate = sibling / ".venv" / "bin" / "sphinx-build"

if candidate.exists():

return str(candidate)

Copilot · 2026-02-08T17:50:08Z

src/ai_memory_protocol/engine.py

+      1. ``workspace/.venv/bin/sphinx-build``
+      2. Walk parent directories for ``.venv/bin/sphinx-build``
+      3. ``shutil.which("sphinx-build")`` (system PATH)
+
+    Returns the path string, or raises ``FileNotFoundError``.
+    """
+    # 1. Workspace venv
+    candidate = workspace / ".venv" / "bin" / "sphinx-build"
+    if candidate.exists():
+        return str(candidate)
+
+    # 2. Walk parent directories
+    for parent in workspace.parents:
+        candidate = parent / ".venv" / "bin" / "sphinx-build"
+        if candidate.exists():
+            return str(candidate)
+        # Also check sibling directories (e.g., ../ros2_medkit/.venv/)
+        if parent.is_dir():
+            for sibling in parent.iterdir():
+                if sibling.is_dir() and sibling != workspace:
+                    candidate = sibling / ".venv" / "bin" / "sphinx-build"
+                    if candidate.exists():
+                        return str(candidate)


The venv candidate path is hard-coded to .venv/bin/sphinx-build, which will not exist on Windows venvs (typically .venv/Scripts/sphinx-build.exe). If this package is intended to be cross-platform, consider checking both bin/ and Scripts/ (and .exe) before falling back to shutil.which.

Suggested change

1. ``workspace/.venv/bin/sphinx-build``

2. Walk parent directories for ``.venv/bin/sphinx-build``

3. ``shutil.which("sphinx-build")`` (system PATH)

Returns the path string, or raises ``FileNotFoundError``.

"""

# 1. Workspace venv

candidate = workspace / ".venv" / "bin" / "sphinx-build"

if candidate.exists():

return str(candidate)

# 2. Walk parent directories

for parent in workspace.parents:

candidate = parent / ".venv" / "bin" / "sphinx-build"

if candidate.exists():

return str(candidate)

# Also check sibling directories (e.g., ../ros2_medkit/.venv/)

if parent.is_dir():

for sibling in parent.iterdir():

if sibling.is_dir() and sibling != workspace:

candidate = sibling / ".venv" / "bin" / "sphinx-build"

if candidate.exists():

return str(candidate)

1. ``workspace/.venv/bin/sphinx-build`` (and Windows equivalents)

2. Walk parent directories for ``.venv/bin/sphinx-build`` (and Windows equivalents)

3. ``shutil.which("sphinx-build")`` (system PATH)

Returns the path string, or raises ``FileNotFoundError``.

"""

def _venv_sphinx_candidate(base: Path) -> str | None:

"""Return the first existing sphinx-build path under base/.venv, or None.

Handles common POSIX and Windows virtualenv layouts.

"""

relative_candidates = [

Path(".venv") / "bin" / "sphinx-build",

Path(".venv") / "Scripts" / "sphinx-build",

Path(".venv") / "Scripts" / "sphinx-build.exe",

]

for rel in relative_candidates:

candidate = base / rel

if candidate.exists():

return str(candidate)

return None

# 1. Workspace venv

venv_path = _venv_sphinx_candidate(workspace)

if venv_path is not None:

return venv_path

# 2. Walk parent directories

for parent in workspace.parents:

venv_path = _venv_sphinx_candidate(parent)

if venv_path is not None:

return venv_path

# Also check sibling directories (e.g., ../ros2_medkit/.venv/)

if parent.is_dir():

for sibling in parent.iterdir():

if sibling.is_dir() and sibling != workspace:

venv_path = _venv_sphinx_candidate(sibling)

if venv_path is not None:

return venv_path

Copilot · 2026-02-08T17:50:08Z

src/ai_memory_protocol/engine.py

+
+    raise FileNotFoundError(
+        "sphinx-build not found. Install it with: pip install sphinx sphinx-needs\n"
+        "Or create a venv in your memory workspace: memory init --install <dir>"


The FileNotFoundError guidance says memory init --install <dir>, but the CLI syntax elsewhere is memory init <dir> --install (dir is the positional argument). Updating this message would prevent users from copy/pasting an invalid command.

Suggested change

"Or create a venv in your memory workspace: memory init --install <dir>"

"Or create a venv in your memory workspace: memory init <dir> --install"

Copilot · 2026-02-08T17:50:08Z

src/ai_memory_protocol/engine.py

+    needs = load_needs(workspace)
+    by_type: dict[str, int] = {}
+    by_status: dict[str, int] = {}
+    for n in needs.values():
+        by_type[n.get("type", "?")] = by_type.get(n.get("type", "?"), 0) + 1
+        by_status[n.get("status", "?")] = by_status.get(n.get("status", "?"), 0) + 1
+


run_rebuild() documents that it “never raises on build failure”, but after a successful Sphinx run it calls load_needs(), which can sys.exit(1) (SystemExit) if needs.json is missing/relocated. To match the contract and avoid crashing CLI/MCP callers, catch SystemExit around load_needs() (and return (False, <message>)) or refactor load_needs() to optionally raise/return an error instead of exiting.

Copilot · 2026-02-08T17:50:09Z

src/ai_memory_protocol/cli.py

+    print(message)
+    if not success:


On failure, the rebuild message is printed to stdout and then the process exits non-zero. Previously failures were sent to stderr; it would be better to keep errors on stderr (and only print success output on stdout) for scripting/pipe use.

Suggested change

print(message)

if not success:

if success:

print(message)

else:

print(message, file=sys.stderr)

Copilot · 2026-02-08T17:50:09Z

src/ai_memory_protocol/mcp_server.py

+            result_lines.append(f"Warning: Memory was added but rebuild failed: {rebuild_msg}")
+            result_lines.append("Run memory_rebuild manually when sphinx-build is available.")


When run_rebuild() returns False it may return messages like Rebuild skipped: ... (e.g., sphinx-build not found). Prefixing it with rebuild failed makes the result misleading/confusing ("failed: skipped"). Consider using a neutral prefix (e.g., "rebuild did not run"), or just append rebuild_msg without re-labeling it.

Suggested change

result_lines.append(f"Warning: Memory was added but rebuild failed: {rebuild_msg}")

result_lines.append("Run memory_rebuild manually when sphinx-build is available.")

result_lines.append(

f"Warning: Memory was added but rebuild did not complete automatically: {rebuild_msg}"

)

result_lines.append("You can run memory_rebuild manually when sphinx-build is available.")

bburda self-assigned this Feb 8, 2026

Copilot AI review requested due to automatic review settings February 8, 2026 17:47

Copilot started reviewing on behalf of bburda February 8, 2026 17:47 View session

bburda merged commit 9be4419 into main Feb 8, 2026
11 checks passed

bburda deleted the fix/sphinx-build-discovery branch February 8, 2026 17:49

Copilot AI reviewed Feb 8, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix sphinx-build discovery and improve rebuild error handling#1

Fix sphinx-build discovery and improve rebuild error handling#1
bburda merged 1 commit intomainfrom
fix/sphinx-build-discovery

bburda commented Feb 8, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 8, 2026

Uh oh!

Copilot AI Feb 8, 2026

Uh oh!

Copilot AI Feb 8, 2026

Uh oh!

Copilot AI Feb 8, 2026

Uh oh!

Copilot AI Feb 8, 2026

Uh oh!

Copilot AI Feb 8, 2026

Uh oh!

Copilot AI Feb 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

	success, message = run_rebuild(workspace)
	_, message = run_rebuild(workspace)

	"Or create a venv in your memory workspace: memory init --install <dir>"
	"Or create a venv in your memory workspace: memory init <dir> --install"

		result_lines.append(f"Warning: Memory was added but rebuild failed: {rebuild_msg}")
		result_lines.append("Run memory_rebuild manually when sphinx-build is available.")

Conversation

bburda commented Feb 8, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant