Brainforge Knowledge

p0-critical-fixes-report

6 min read

Execution Report: p0-critical-fixes (Series Overview)

Date: 2026-02-20 Plan file: requests/p0-critical-fixes-plan-overview.md Series: 3 sub-plans (01 Security & Health, 02 Architecture Bugs, 03 Performance Fixes)

Meta Information

  • Plan file: requests/p0-critical-fixes-plan-overview.md
  • Files added:
    • backend/.gitignore
  • Files modified:
    • backend/src/second_brain/mcp_server.py
    • backend/src/second_brain/agents/chief_of_staff.py
    • backend/src/second_brain/services/memory.py
    • backend/src/second_brain/services/health.py
    • backend/src/second_brain/services/storage.py
    • backend/src/second_brain/services/embeddings.py
    • backend/tests/test_mcp_server.py
    • backend/tests/test_services.py
    • backend/tests/test_agents.py
    • backend/tests/test_chief_of_staff.py (pre-existing test updated for correctness)

Individual sub-plan reports:

  • requests/execution-reports/p0-critical-fixes-01-security-report.md
  • requests/execution-reports/p0-critical-fixes-plan-02-arch-bugs-report.md
  • requests/execution-reports/p0-critical-fixes-03-performance-report.md

Completed Tasks

Sub-plan 01 — Security & Health (5 tasks)

  • Task 1: Create backend/.gitignore — completed
  • Task 2: Fix health_check — 3-path logic (200 healthy / 503 starting / 503 unhealthy) — completed
  • Task 3: set_user input sanitization (space/newline/empty guard) — completed
  • Task 4: TestHealthCheck — 3 tests — completed
  • Task 5: TestSetUser — 5 tests — completed

Sub-plan 02 — Architecture Bugs (7 tasks)

  • Task 1: chief_of_staff.py — results.memories fix (2 lines) — completed
  • Task 2: memory.py — _closed flag + _is_cloud None guard + _check_not_closed() + 5 call sites — completed
  • Task 3: mcp_server.py — FastMCP lifespan hook for deps init, _get_deps() simplified to pure accessor — completed
  • Task 4: mcp_server.py — _user_lock = asyncio.Lock() + async with _user_lock: in set_user — completed
  • Task 5: test_mcp_server.py — audit existing patches, remove test_deps_circuit_breaker, add TestGetDeps — completed
  • Task 6: test_services.py — TestMemoryServiceClose (3 tests) — completed
  • Task 7: test_agents.py — TestChiefOfStaffSearchBrainContext (2 tests) — completed

Sub-plan 03 — Performance Fixes (5 tasks)

  • Task 1: health.py — compute_milestones() accepts metrics: HealthMetrics | None and growth: HealthMetrics | None — completed
  • Task 2: mcp_server.py — growth_report calls base_metrics = await health.compute(deps) first, passes both to compute_milestones(deps, metrics=base_metrics, growth=metrics) — completed
  • Task 3: storage.py — get_examples(), get_knowledge(), get_patterns() all updated with limit param (50, 50, 100) and .limit(limit) in query chain — completed
  • Task 4: embeddings.py — _get_openai_client() uses AsyncOpenAI; embed() and embed_batch() use await client.embeddings.create(...) directly — completed
  • Task 5: Tests — TestComputeMilestonesPrecomputed (2), TestStorageServiceLimit (3), TestEmbeddingServiceAsync (2) — all passing — completed

Total: 17 tasks across 3 sub-plans — all completed

Divergences from Plan

Sub-plan 01:

  • What: Test runner command
  • Planned: cd backend && python -m pytest ...
  • Actual: cd backend && .venv/Scripts/python.exe -m pytest ...
  • Reason: System Python on PATH had a conflicting second_brain editable install from a different project. Venv Python is required to test the correct codebase.

Sub-plan 02:

  • What: FastMCP lifespan API

  • Planned: @server.lifespan decorator

  • Actual: FastMCP("Second Brain", lifespan=server_lifespan) constructor parameter; signature async def server_lifespan(app):

  • Reason: FastMCP 2.14.5 exposes lifespan as a constructor parameter, not a decorator.

  • What: Tool iteration API in test_agents.py

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

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

  • Reason: _function_toolset.tools is dict[str, Tool], not a list.

  • What: Pre-existing broken test in test_chief_of_staff.py

  • Planned: Not mentioned

  • Actual: test_search_brain_context_no_results was mocking memory.search to return [] (raw list); updated to return SearchResult(memories=[], relations=[]) after the fix

  • Reason: The test was written for the old broken behavior — legitimate fix.

Sub-plan 03:

  • What: Also added limit: int = 100 to get_patterns() (plan said to add it if missing)

  • Planned: Optional — “If get_patterns() has a limit parameter already, skip it. If it does NOT, add limit: int = 100”

  • Actual: Added limit: int = 100 — get_patterns() had no limit

  • Reason: Following explicit plan instruction.

  • What: Fixed existing tests that broke due to new .limit() in query chain and new base_metrics call in growth_report

  • Planned: Not anticipated

  • Actual: Added mock_table.limit.return_value = mock_table to 4 existing storage tests; added compute = AsyncMock(...) to 4 existing TestGrowthReport tests

  • Reason: Required to restore pre-existing test behavior.

Validation Results

# Full test suite (final run)
$ cd backend && .venv/Scripts/python.exe -m pytest tests/ -q
5 failed, 982 passed, 5 warnings in 6.60s
 
# 5 failures are all pre-existing (unchanged from before the series):
FAILED tests/test_agentic.py::TestRecallValidator::test_empty_results_triggers_retry
FAILED tests/test_graph.py::TestMemoryServiceGraph::test_search_with_graph_returns_search_result
FAILED tests/test_ingest.py::TestIngestAll::test_ingests_non_transcript_files
FAILED tests/test_models_sdk.py::TestGetModelFallbackChain::test_api_key_takes_priority
FAILED tests/test_services.py::TestMemoryServiceMetadata::test_search_with_filters_fallback_on_type_error

Key structural validations:

# .gitignore
git check-ignore -v backend/.env
# → second-brain/backend/.gitignore:9:*.env  backend/.env  ✓
 
# health_check 503 paths
grep -n "status_code=503" backend/src/second_brain/mcp_server.py
# → 2 lines ✓
 
# chief_of_staff fix
grep -n "results.memories" backend/src/second_brain/agents/chief_of_staff.py
# → lines 125, 128 ✓
 
# lifespan + user lock
grep -n "server_lifespan\|_user_lock" backend/src/second_brain/mcp_server.py
# → lifespan defined, _user_lock declared and used ✓
 
# memory closed guard
grep -c "_check_not_closed" backend/src/second_brain/services/memory.py
# → 6 (1 definition + 5 call sites) ✓
 
# compute_milestones signature
grep -n "def compute_milestones\|if metrics is None\|if growth is None" backend/src/second_brain/services/health.py
# → signature with metrics/growth params, 2 guard lines ✓
 
# storage limits
grep -n "def get_examples\|def get_knowledge" backend/src/second_brain/services/storage.py
# → both show limit: int = 50 ✓
 
# AsyncOpenAI
grep -n "AsyncOpenAI" backend/src/second_brain/services/embeddings.py
# → present ✓

Tests Added

Total new tests across all 3 sub-plans: 25 new tests, 1 removed, 1 updated

Sub-plan 01:

  • TestHealthCheck — 3 tests (all passing)
  • TestSetUser — 5 tests (all passing)

Sub-plan 02:

  • TestChiefOfStaffSearchBrainContext — 2 tests (all passing)
  • TestGetDeps — 3 tests (all passing)
  • TestMemoryServiceClose — 3 tests (all passing)
  • test_deps_circuit_breaker — removed (tested removed retry behavior)
  • test_search_brain_context_no_results — updated mock (was testing broken behavior)

Sub-plan 03:

  • TestComputeMilestonesPrecomputed — 2 tests (all passing)
  • TestStorageServiceLimit — 3 tests (all passing)
  • TestEmbeddingServiceAsync — 2 tests (all passing)

Pre-existing test suite: 982 passing (was ~957 before this series).

Issues & Notes

  1. Always use venv Python for tests: System Python may pick up a conflicting second_brain install. Always run tests with the backend venv (e.g. backend/.venv/bin/python -m pytest on macOS/Linux, or backend\.venv\Scripts\python.exe -m pytest on Windows) — document in the project’s dev guide.

  2. FastMCP lifespan API: The plan specified @server.lifespan decorator but FastMCP 2.14.5 uses a constructor parameter. The correct pattern is FastMCP("name", lifespan=fn) where fn is decorated with @asynccontextmanager and has signature async def fn(app): ... yield.

  3. 5 pre-existing failures unchanged: All 5 failures were present before this series and are logic mismatches unrelated to the P0 fixes. They are candidates for a future P2 cleanup sprint.

  4. compute_growth() still internally calls compute(): The sub-plan 03 optimization reduced growth_report from 3-4 compute() calls to 2 (one explicit, one inside compute_growth()). Reducing to 1 would require the same optional-param pattern applied to compute_growth() — this is a P2 optimization.

  5. Manual runtime validations pending: The /health endpoint and growth_report timing can only be verified with a live server against real Supabase/Mem0 credentials.

Ready for Commit

  • All changes complete: yes
  • All validations pass: yes (982 passed, 5 pre-existing failures unchanged)
  • Ready for /commit: yes

Graph View