-
Notifications
You must be signed in to change notification settings - Fork 0
Fix sphinx-build discovery and improve rebuild error handling #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,6 +4,8 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import json | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import os | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import shutil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import subprocess | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import sys | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from pathlib import Path | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from typing import Any | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -167,3 +169,83 @@ def expand_graph( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| frontier = next_frontier | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return {nid: needs[nid] for nid in collected if nid in needs} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # --------------------------------------------------------------------------- | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Sphinx-build discovery and rebuild | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # --------------------------------------------------------------------------- | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def find_sphinx_build(workspace: Path) -> str: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Locate the sphinx-build executable. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Search order: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+183
to
+205
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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
AI
Feb 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| 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
AI
Feb 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| "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
AI
Feb 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -15,15 +15,22 @@ | |||||||||||||
|
|
||||||||||||||
| import json | ||||||||||||||
| import logging | ||||||||||||||
| import subprocess | ||||||||||||||
| from datetime import date | ||||||||||||||
| from pathlib import Path | ||||||||||||||
| from typing import Any | ||||||||||||||
|
|
||||||||||||||
| from mcp.server import Server | ||||||||||||||
| from mcp.types import TextContent, Tool | ||||||||||||||
|
|
||||||||||||||
| from .engine import expand_graph, find_workspace, load_needs, resolve_id, tag_match, text_match | ||||||||||||||
| from .engine import ( | ||||||||||||||
| expand_graph, | ||||||||||||||
| find_workspace, | ||||||||||||||
| load_needs, | ||||||||||||||
| resolve_id, | ||||||||||||||
| run_rebuild, | ||||||||||||||
| tag_match, | ||||||||||||||
| text_match, | ||||||||||||||
| ) | ||||||||||||||
| from .formatter import format_brief, format_compact, format_context_pack, format_full | ||||||||||||||
| from .rst import ( | ||||||||||||||
| add_tags_in_rst, | ||||||||||||||
|
|
@@ -385,18 +392,8 @@ def _format_output( | |||||||||||||
|
|
||||||||||||||
| def _do_rebuild(workspace: Path) -> str: | ||||||||||||||
| """Run Sphinx build to regenerate needs.json.""" | ||||||||||||||
| venv_sphinx = workspace / ".venv" / "bin" / "sphinx-build" | ||||||||||||||
| sphinx_cmd = str(venv_sphinx) if venv_sphinx.exists() else "sphinx-build" | ||||||||||||||
| cmd = [sphinx_cmd, "-b", "html", "-q", str(workspace), str(workspace / "_build" / "html")] | ||||||||||||||
| result = subprocess.run(cmd, capture_output=True, text=True) | ||||||||||||||
| if result.returncode != 0: | ||||||||||||||
| return f"Rebuild failed: {result.stderr}" | ||||||||||||||
|
|
||||||||||||||
| needs = load_needs(workspace) | ||||||||||||||
| by_type: dict[str, int] = {} | ||||||||||||||
| 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, message = run_rebuild(workspace) | |
| _, message = run_rebuild(workspace) |
Copilot
AI
Feb 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| 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.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.