fix: address PR review comments and CI formatting issues

- capture.py: Use record/field separators (0x1e/0x1f) for git log parsing
  to handle multi-line commit bodies safely; fix since/until handling to
  distinguish date expressions from ref ranges; add explanatory comment
  on bare except clause
- executor.py: Check returncode in _git_stash_push before inspecting
  stdout; treat build failure as unsuccessful regardless of stash state;
  set success=False when any actions failed
- planner.py: Set field_changes={'status':'review'} in detect_conflicts
  instead of empty dict; raise ValueError for unknown check names
- mcp_server.py: Fix pipx install syntax from 'ai_memory_protocol/[mcp]'
  to '.[mcp]'
- cli.py: Remove unnecessary lambda wrappers around no-arg callables
- ci.yml: Add --cov-report=html for htmlcov/ artifact upload; add
  pytest-timeout to integration test dependencies
- Run ruff format on all files to fix formatting CI failure

Author: bburda

.github/workflows/ci.yml

@@ -64,7 +64,7 @@ jobs:
           python -c "from ai_memory_protocol.mcp_server import create_mcp_server; create_mcp_server()"
 
       - name: Run unit tests
-        run: pytest tests/ -v -m "not integration" --cov=ai_memory_protocol --cov-report=term-missing
+        run: pytest tests/ -v -m "not integration" --cov=ai_memory_protocol --cov-report=term-missing --cov-report=html
 
       - name: Upload coverage
         if: matrix.python-version == '3.12'
@@ -85,7 +85,7 @@ jobs:
           python-version: "3.12"
 
       - name: Install package
-        run: pip install -e '.[mcp]' pytest
+        run: pip install -e '.[mcp]' pytest pytest-timeout
 
       - name: Run integration tests
         run: pytest tests/ -v -m integration --timeout=120

src/ai_memory_protocol/capture.py

@@ -56,8 +56,11 @@ def to_dict(self) -> dict[str, Any]:
 # ---------------------------------------------------------------------------
 
 
-_GIT_LOG_SEP = "---MEMORY_SEP---"
-_GIT_LOG_FORMAT = f"%H{_GIT_LOG_SEP}%s{_GIT_LOG_SEP}%b{_GIT_LOG_SEP}%an{_GIT_LOG_SEP}%ad"
+_GIT_RECORD_SEP = "\x1e"  # ASCII Record Separator between commits
+_GIT_FIELD_SEP = "\x1f"  # ASCII Unit Separator between fields
+_GIT_LOG_FORMAT = (
+    f"%H{_GIT_FIELD_SEP}%s{_GIT_FIELD_SEP}%b{_GIT_FIELD_SEP}%an{_GIT_FIELD_SEP}%ad{_GIT_RECORD_SEP}"
+)
 
 
 @dataclass
@@ -74,35 +77,40 @@ class _GitCommit:
 
 def _parse_git_log(repo_path: Path, since: str, until: str) -> list[_GitCommit]:
     """Run git log and parse the output."""
-    cmd = [
+    # Base git log command
+    cmd: list[str] = [
         "git",
         "log",
         f"--format={_GIT_LOG_FORMAT}",
         "--date=iso-strict",
-        f"{since}..{until}" if ".." not in since and until != "HEAD" else f"{since}...{until}",
     ]
 
-    # Use simpler range format
-    if since and until:
-        cmd = [
-            "git",
-            "log",
-            f"--format={_GIT_LOG_FORMAT}",
-            "--date=iso-strict",
-            f"--since={since}" if not since.startswith("HEAD") else "",
-            f"--until={until}" if not until.startswith("HEAD") else "",
-        ]
-        cmd = [c for c in cmd if c]  # Remove empty strings
-
-    # Fallback: use commit range directly
-    if since.startswith("HEAD"):
-        cmd = [
-            "git",
-            "log",
-            f"--format={_GIT_LOG_FORMAT}",
-            "--date=iso-strict",
-            f"{since}",
-        ]
+    # Heuristic: if arguments contain spaces (e.g. "2 weeks ago"), treat them as
+    # date expressions and use --since/--until. Otherwise treat them as refs and
+    # use git's revision range syntax.
+    has_since = bool(since)
+    has_until = bool(until)
+    since_is_date = has_since and (" " in since)
+    until_is_date = has_until and (" " in until)
+
+    if has_since and has_until:
+        if since_is_date or until_is_date:
+            # Date-based range
+            cmd.append(f"--since={since}")
+            cmd.append(f"--until={until}")
+        else:
+            # Ref-based range: use {since}..{until}
+            cmd.append(f"{since}..{until}")
+    elif has_since:
+        if since_is_date:
+            cmd.append(f"--since={since}")
+        else:
+            cmd.append(since)
+    elif has_until:
+        if until_is_date:
+            cmd.append(f"--until={until}")
+        else:
+            cmd.append(until)
 
     try:
         result = subprocess.run(
@@ -117,10 +125,12 @@ def _parse_git_log(repo_path: Path, since: str, until: str) -> list[_GitCommit]:
         return []
 
     commits: list[_GitCommit] = []
-    for line in result.stdout.strip().split("\n"):
-        if not line.strip():
+    # Split on record separator — safe even when %b contains newlines
+    for record in result.stdout.split(_GIT_RECORD_SEP):
+        record = record.strip()
+        if not record:
             continue
-        parts = line.split(_GIT_LOG_SEP)
+        parts = record.split(_GIT_FIELD_SEP)
         if len(parts) < 5:
             continue
         commit = _GitCommit(
@@ -143,6 +153,9 @@ def _parse_git_log(repo_path: Path, since: str, until: str) -> list[_GitCommit]:
             )
             c.files = [f.strip() for f in files_result.stdout.strip().split("\n") if f.strip()]
         except OSError:
+            # Best-effort: if git diff-tree fails for this commit (e.g. git not
+            # available or repository in an unexpected state), leave the files
+            # list unchanged for this commit and continue processing others.
             pass
 
     return commits
@@ -418,7 +431,7 @@ def capture_from_git(
                 title=title[:120],
                 body="\n".join(body_parts),
                 tags=sorted(all_tags),
-                source=f"commit:{primary.hash[:8]}+{len(group)-1}",
+                source=f"commit:{primary.hash[:8]}+{len(group) - 1}",
                 confidence=confidence,
                 scope=f"repo:{repo_name}",
                 _source_hashes=[c.hash for c in group],

src/ai_memory_protocol/cli.py

@@ -151,6 +151,7 @@ def _check_rst_files(workspace_dir: str | None) -> tuple[bool, str]:
         return False, f"{len(errors)} unreadable files: {'; '.join(errors)}"
     return True, f"{len(rst_files)} RST files OK"
 
+
 # ---------------------------------------------------------------------------
 # Subcommands
 # ---------------------------------------------------------------------------
@@ -171,12 +172,12 @@ def cmd_doctor(args: argparse.Namespace) -> None:
     """Run installation health checks."""
     ws_dir = getattr(args, "dir", None)
     checks = [
-        ("CLI entry point", lambda: _check_cli()),
+        ("CLI entry point", _check_cli),
         ("Workspace exists", lambda: _check_workspace(ws_dir)),
         ("Sphinx-build available", lambda: _check_sphinx_build(ws_dir)),
         ("needs.json loadable", lambda: _check_needs_json(ws_dir)),
-        ("MCP SDK installed", lambda: _check_mcp_importable()),
-        ("MCP server creatable", lambda: _check_mcp_server()),
+        ("MCP SDK installed", _check_mcp_importable),
+        ("MCP server creatable", _check_mcp_server),
         ("RST files parseable", lambda: _check_rst_files(ws_dir)),
     ]
     all_ok = True

src/ai_memory_protocol/executor.py

@@ -120,10 +120,12 @@ def validate_actions(actions: list[Action]) -> tuple[list[Action], list[dict[str
                 visited.add(current)
                 current = supersede_map[current]
             if cycle:
-                skipped.append({
-                    "action": a.to_dict(),
-                    "reason": f"Circular supersede chain involving {a.old_id}",
-                })
+                skipped.append(
+                    {
+                        "action": a.to_dict(),
+                        "reason": f"Circular supersede chain involving {a.old_id}",
+                    }
+                )
                 continue
 
         valid.append(a)
@@ -213,8 +215,7 @@ def _execute_split_file(workspace: Path, action: Action) -> tuple[bool, str]:  #
     via rst.py append_to_rst when MAX_ENTRIES_PER_FILE is exceeded.
     """
     return True, (
-        f"File splitting noted for {action.rst_path}"
-        " — handled automatically on next append."
+        f"File splitting noted for {action.rst_path} — handled automatically on next append."
     )
 
 
@@ -243,8 +244,13 @@ def _git_stash_push(workspace: Path) -> bool:
             capture_output=True,
             text=True,
         )
-        # "No local changes to save" means nothing was stashed
-        return "No local changes" not in result.stdout
+        # Only treat as successful if git exited cleanly.
+        if result.returncode != 0:
+            return False
+        # "No local changes to save" means nothing was stashed.
+        # This may appear in stdout or stderr.
+        output = (result.stdout or "") + (result.stderr or "")
+        return "No local changes to save" not in output
     except OSError:
         return False
 
@@ -346,10 +352,12 @@ def execute_plan(
     for action in valid_actions:
         executor = _EXECUTORS.get(action.kind)
         if not executor:
-            failed.append({
-                "action": action.to_dict(),
-                "error": f"Unknown action kind: {action.kind}",
-            })
+            failed.append(
+                {
+                    "action": action.to_dict(),
+                    "error": f"Unknown action kind: {action.kind}",
+                }
+            )
             continue
 
         try:
@@ -368,16 +376,28 @@ def execute_plan(
     if rebuild and applied:
         build_ok, build_output = run_rebuild(workspace)
 
-    # If build failed, rollback
-    if not build_ok and stashed:
-        _git_stash_pop(workspace)
+    # If build failed, always treat as unsuccessful; use git stash for rollback
+    # when available.
+    if not build_ok:
+        if stashed:
+            _git_stash_pop(workspace)
+            applied_result: list[dict[str, Any]] = []
+            message = "Build failed after applying actions — rolled back via git stash pop."
+        else:
+            # No stash available: cannot automatically roll back workspace changes.
+            applied_result = applied
+            message = (
+                "Build failed after applying actions — no git stash available for "
+                "rollback; workspace may be in an inconsistent state."
+            )
+
         return ExecutionResult(
             success=False,
-            applied=[],
+            applied=applied_result,
             failed=failed,
             skipped=skipped,
             build_output=build_output,
-            message="Build failed after applying actions — rolled back via git stash pop.",
+            message=message,
         )
 
     # Cleanup stash on success
@@ -390,15 +410,15 @@ def execute_plan(
         msg = f"memory: auto-apply {', '.join(sorted(kinds))} ({len(applied)} actions)"
         _git_commit(workspace, msg)
 
+    all_succeeded = not failed
     return ExecutionResult(
-        success=True,
+        success=all_succeeded,
         applied=applied,
         failed=failed,
         skipped=skipped,
         build_output=build_output,
         message=(
-            f"Plan executed: {len(applied)} applied, "
-            f"{len(failed)} failed, {len(skipped)} skipped."
+            f"Plan executed: {len(applied)} applied, {len(failed)} failed, {len(skipped)} skipped."
         ),
     )