Brainforge Knowledge

p0-critical-fixes-plan-02-arch-bugs-report

4 min read

Execution Report: p0-critical-fixes-plan-02-arch-bugs

Date: 2026-02-20 Plan file: requests/p0-critical-fixes-plan-02-arch-bugs.md

Meta Information

  • Plan file: requests/p0-critical-fixes-plan-02-arch-bugs.md
  • Files added: None
  • Files modified:
    • backend/src/second_brain/agents/chief_of_staff.py
    • backend/src/second_brain/services/memory.py
    • backend/src/second_brain/mcp_server.py
    • backend/tests/test_mcp_server.py
    • backend/tests/test_services.py
    • backend/tests/test_agents.py
    • backend/tests/test_chief_of_staff.py (bonus fix — pre-existing test used wrong mock type)

Completed Tasks

  • Task 1: chief_of_staff.py — results.memories fix — completed (2 lines changed)
  • Task 2: memory.py — _closed flag + _is_cloud None guard + _check_not_closed + 5 call sites — completed
  • Task 3: mcp_server.py — lifespan hook added, _get_deps() simplified — completed
  • Task 4: mcp_server.py — _user_lock + set_user lock acquisition — completed
  • Task 5: test_mcp_server.py — test_deps_circuit_breaker removed (tests removed behavior), TestGetDeps added — completed
  • Task 6: test_services.py — TestMemoryServiceClose added — completed
  • Task 7: test_agents.py — TestChiefOfStaffSearchBrainContext added — completed

Divergences from Plan

  • What: FastMCP lifespan API

  • Planned: @server.lifespan decorator pattern

  • Actual: FastMCP("Second Brain", lifespan=server_lifespan) constructor parameter; function signature is async def server_lifespan(app): (takes the app as argument)

  • Reason: FastMCP 2.14.5 exposes lifespan as a constructor parameter (Callable[[FastMCP], AsyncContextManager]), not as a @server.lifespan decorator. The @asynccontextmanager wrapper is still used.

  • What: Pre-existing test broken by the fix

  • Planned: Not mentioned in plan

  • Actual: test_chief_of_staff.py::test_search_brain_context_no_results was mocking memory.search to return [] (raw list); after the fix accessing results.memories raises AttributeError on a list. Updated the mock to return SearchResult(memories=[], relations=[]).

  • Reason: The test was written for the old broken behavior — this is a legitimate fix, not a regression.

  • What: __main__ pre-init block removed

  • Planned: Not explicitly mentioned — plan only said create_deps() should only appear in server_lifespan

  • Actual: Removed the pre-initialization block in if __name__ == "__main__": that also called create_deps() before server.run()

  • Reason: The plan validation check requires create_deps() ONLY in server_lifespan. The lifespan hook now handles initialization.

  • What: Tool lookup API in test_agents.py

  • Planned: for tool in chief_of_staff._function_toolset.tools: if tool.name == "search_brain_context"

  • Actual: chief_of_staff._function_toolset.tools.get("search_brain_context") (dict lookup by name)

  • Reason: _function_toolset.tools is a dict[str, Tool], not a list. Iterating gives string keys, not Tool objects.

Validation Results

# Level 1: Syntax & Structure
$ grep -n "results.memories" backend/src/second_brain/agents/chief_of_staff.py
125:        if not results.memories:
128:        return format_memories(results.memories, limit=5)
 
$ grep -n "format_memories(results," backend/src/second_brain/agents/chief_of_staff.py
# (0 lines — correct)
 
$ grep -n "@server.lifespan|server_lifespan|asynccontextmanager|create_deps|_user_lock" backend/src/second_brain/mcp_server.py
9:from contextlib import asynccontextmanager
19:from second_brain.deps import BrainDeps, create_deps
47:@asynccontextmanager
48:async def server_lifespan(app):
54:        _deps = create_deps()
98:_user_lock = asyncio.Lock()
146:    async with _user_lock:
 
$ grep -n "_closed|_check_not_closed" backend/src/second_brain/services/memory.py
# 9 lines: init, 5 call sites, 1 definition, closed setter, is_cloud guard
 
# Level 2: Unit Tests
$ python -m pytest tests/test_agents.py::TestChiefOfStaffSearchBrainContext -v
PASSED (2/2)
 
$ python -m pytest tests/test_mcp_server.py::TestGetDeps -v
PASSED (3/3)
 
$ python -m pytest tests/test_services.py::TestMemoryServiceClose -v
PASSED (3/3)
 
# Level 3: Full Test Suite
$ python -m pytest tests/ -q
5 failed, 975 passed (5 pre-existing failures, 0 regressions)
 
# Level 4: Manual Validation — all PASS
SearchResult.memories: [{'x': 1}]
_is_cloud with None: False
_get_deps() raises RuntimeError: "Second Brain not yet initialized..."

Tests Added

  • backend/tests/test_agents.py::TestChiefOfStaffSearchBrainContext — 2 tests (pass)
  • backend/tests/test_mcp_server.py::TestGetDeps — 3 tests (pass)
  • backend/tests/test_services.py::TestMemoryServiceClose — 3 tests (pass)
  • backend/tests/test_chief_of_staff.py::test_search_brain_context_no_results — updated mock (was broken by fix, now fixed)
  • backend/tests/test_mcp_server.py::test_deps_circuit_breaker — removed (tested removed behavior)

Total new: 8 tests added, 1 removed, 1 updated.

Issues & Notes

  • The FastMCP lifespan API differs from the plan’s example. The correct pattern for FastMCP >=2.14.0 is to pass lifespan=fn to the constructor, where fn is a Callable[[FastMCP], AsyncContextManager]. The @asynccontextmanager decorator is still required.
  • The existing test_chief_of_staff.py::test_search_brain_context_no_results was a latent bug in the test suite — it was testing the OLD broken behavior where search() returned a raw list. Updated to use SearchResult which is correct.
  • 5 pre-existing test failures remain unchanged (same tests that were failing before this sub-plan).

Ready for Commit

  • All changes complete: yes
  • All validations pass: yes
  • Ready for /commit: yes

Graph View