Split marm-console's server/app.py into models + core + endpoints/ - #99
Conversation
Pure refactor, no behavior changes, no route/path/method changes. Mirrors marm-mcp-server's own core/models.py + endpoints/*.py + router registration convention (confirmed with user over a lighter grouping). Extracted via exact index-based line-slicing -- all 15 pieces (including 4 non-contiguous ones) verified byte-identical to the original before any import cleanup. - server/app.py keeps only app wiring: CORS, startup event, /health, and 8 app.include_router(...) calls. 915 -> 74 lines. - server/models.py (new): all 15 Pydantic payload models. - server/core.py (new): helpers genuinely shared across 2+ domains -- _now_iso, get_memory_db_path, get_concept_db_path, _concepts_payload, _mcp_tool_mutation. - server/endpoints/ (new package, 8 files): overview, memory, sessions, logs, notebook, compaction, concepts, projects. Each owns its domain's routes plus any helper used only within that domain (_memory_mutation stays in memory.py, _project_operation stays in projects.py, concept-build job-tracking state stays in concepts.py). Deliberate departure from a literal 1:1 mirror: /health, CORS, and the startup event stay in app.py rather than getting their own endpoints/system.py -- one 3-line endpoint with zero shared deps would be exactly the "tiny module with no real boundary" refactor-middle- ground.md warns against. Two real bugs caught during implementation, both by tooling working as designed: - ruff check caught endpoints/concepts.py calling _concepts_payload without importing it from core.py. - pytest caught a gap ruff couldn't: test_memory_dashboard_gap_routes.py monkeypatched console_app.memory_store directly (4 call sites), which broke once app.py correctly stopped importing memory_store itself. Repointed to import memory_store directly from server -- also fixed test_concept_store.py's console_app._stale_build_result call the same way, since that function moved to endpoints/concepts.py. Validated: ruff check + ruff format --check clean, full suite 18 passed / 0 failed (matches baseline), and a live TestClient smoke pass confirmed all 8 domain routers plus /health are correctly mounted (no silent 404s from a missing include_router call).
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe Console REST implementation was split from ChangesConsole API refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@marm-console/server/endpoints/compaction.py`:
- Around line 35-46: Replace the list_compaction-based candidate lookup in the
action == "stage" path with a direct parameterized lookup by candidate_id that
does not apply the 200-row limit. Preserve the existing
MemoryStoreUnavailable-to-503 handling and return the same 404 response when the
direct lookup finds no candidate.
In `@marm-console/server/endpoints/concepts.py`:
- Around line 21-28: Synchronize all accesses to the global
_launching_concept_builds dictionary with a threading.Lock: define
_launching_concept_builds_lock and guard pruning in
_prune_launching_concept_builds; in marm-console/server/endpoints/concepts.py
lines 124-142, protect new-job assignment; in lines 156-163, protect the
background thread’s .get() and inner update; and in lines 171-172, guard the
get_concept_build lookup.
In `@marm-console/server/endpoints/logs.py`:
- Around line 58-68: Update the log-deletion loop using the same
failure-collection pattern as delete_all_sessions: continue processing remaining
logs when _mcp_tool_mutation fails, record each failed log ID, and return a
partial_success response containing the affected IDs alongside the successful
deletion counts so callers can reconcile or retry.
In `@marm-console/server/endpoints/memory.py`:
- Around line 52-60: The _memory_mutation function must translate MCP response
errors before returning: reuse the response-status mapping established by
server/core.py:_mcp_tool_mutation for responses with status "error" or
"not_found", while preserving successful mutation responses and existing
exception handling.
In `@marm-console/server/endpoints/overview.py`:
- Around line 5-9: Replace the direct health-probe request in the overview
endpoint with the shared mcp_client.get("health", timeout=1.5) call, removing
the related urllib/json handling. Validate and use the client’s dictionary
response, and catch the mcp_client client exceptions so probe failures return
{"reachable": False} instead of propagating or producing a 500.
In `@marm-console/server/endpoints/projects.py`:
- Around line 27-34: Update the index_project endpoint to use the existing
_project_operation helper instead of calling mcp_client.post directly,
preserving the "internal/projects/index" operation and payload.model_dump()
arguments so inline MCP error payloads map to HTTP 503 consistently.
- Around line 19-24: Update get_projects to catch mcp_client.McpRequestError in
addition to McpUnavailable, mapping it to the same appropriate HTTPException
response pattern used by the other endpoints so request failures do not become
unhandled HTTP 500 errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a8a8b1e2-083c-4f62-8f27-27261cf905ea
📒 Files selected for processing (14)
marm-console/server/app.pymarm-console/server/core.pymarm-console/server/endpoints/__init__.pymarm-console/server/endpoints/compaction.pymarm-console/server/endpoints/concepts.pymarm-console/server/endpoints/logs.pymarm-console/server/endpoints/memory.pymarm-console/server/endpoints/notebook.pymarm-console/server/endpoints/overview.pymarm-console/server/endpoints/projects.pymarm-console/server/endpoints/sessions.pymarm-console/server/models.pymarm-console/tests/test_concept_store.pymarm-console/tests/test_memory_dashboard_gap_routes.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Prioritize runtime correctness, async/concurrency safety, SQLite transaction safety, auth/rate-limit behavior, release-breaking packaging issues, and MCP protocol compatibility.
Files:
marm-console/server/endpoints/__init__.pymarm-console/server/models.pymarm-console/server/endpoints/logs.pymarm-console/server/endpoints/notebook.pymarm-console/server/endpoints/sessions.pymarm-console/tests/test_concept_store.pymarm-console/server/endpoints/compaction.pymarm-console/server/endpoints/overview.pymarm-console/server/endpoints/memory.pymarm-console/tests/test_memory_dashboard_gap_routes.pymarm-console/server/core.pymarm-console/server/endpoints/projects.pymarm-console/server/endpoints/concepts.pymarm-console/server/app.py
**/tests/**
⚙️ CodeRabbit configuration file
**/tests/**: Focus on tests that are flaky, non-isolated, incorrectly asserting behavior, or missing coverage for a changed high-risk path. Skip minor naming, comments, and layout preferences.
Files:
marm-console/tests/test_concept_store.pymarm-console/tests/test_memory_dashboard_gap_routes.py
🪛 ast-grep (0.44.1)
marm-console/server/endpoints/overview.py
[warning] 24-24: Request-controlled URL passed to urlopen; validate against an allowlist to prevent SSRF.
Context: urlopen(url, timeout=1.5)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(urlopen-unsanitized-data)
🔇 Additional comments (12)
marm-console/server/core.py (1)
1-63: LGTM!marm-console/server/models.py (1)
1-91: LGTM!marm-console/server/endpoints/__init__.py (1)
1-2: LGTM!marm-console/server/app.py (1)
13-26: LGTM!Also applies to: 49-56
marm-console/server/endpoints/overview.py (1)
1-4: LGTM!Also applies to: 11-16, 39-42, 45-71
marm-console/server/endpoints/memory.py (1)
14-49: LGTM!Also applies to: 63-66
marm-console/server/endpoints/sessions.py (1)
14-84: LGTM!marm-console/server/endpoints/logs.py (1)
14-46: LGTM!marm-console/server/endpoints/notebook.py (1)
14-72: LGTM!marm-console/server/endpoints/compaction.py (1)
15-28: LGTM!marm-console/tests/test_memory_dashboard_gap_routes.py (1)
8-8: LGTM!Also applies to: 93-93, 110-115, 129-129
marm-console/tests/test_concept_store.py (1)
11-11: LGTM!Also applies to: 300-305
| import json | ||
| import os | ||
| import time | ||
| from urllib.error import URLError | ||
| from urllib.request import urlopen |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the shared MCP client for the health probe.
This direct request drops the MARM_API_KEY header and skips dictionary-response validation provided by mcp_client.get(). Authenticated MCP instances appear unreachable, while non-object JSON causes /api/overview to return 500. Use mcp_client.get("health", timeout=1.5) and map its client exceptions to {"reachable": False}.
Also applies to: 19-36
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@marm-console/server/endpoints/overview.py` around lines 5 - 9, Replace the
direct health-probe request in the overview endpoint with the shared
mcp_client.get("health", timeout=1.5) call, removing the related urllib/json
handling. Validate and use the client’s dictionary response, and catch the
mcp_client client exceptions so probe failures return {"reachable": False}
instead of propagating or producing a 500.
All 7 findings were confirmed real, pre-existing bugs in the original
app.py (carried over verbatim by the split). Fixing here since this
branch is the only place with easy access to the touched code right now.
- compaction.py: stage lookup now queries by ID directly instead of
searching list_compaction's 200-row window (older candidates 404'd).
- concepts.py: lock _launching_concept_builds against concurrent
request-thread access (unsynchronized dict mutation/iteration).
- logs.py: bulk log delete now collects per-log failures and returns
partial_success instead of aborting the whole loop on the first error.
- memory.py: _memory_mutation now maps inline {status: error/not_found}
responses to HTTP errors instead of returning 200/201 on failure.
- overview.py: health probe now goes through the shared mcp_client
(carries MARM_API_KEY) instead of a bare urlopen call that dropped it.
- projects.py: get_projects() catches McpRequestError; index_project()
uses the existing _project_operation helper so inline error payloads
map to 503 instead of a false 202.
8 new tests, one per real behavior change, plus a thread-contention
stress test for the concept-build lock. Every fix was mutation-tested:
reverted individually and confirmed its new test fails, then restored.
Full suite: 27 passed, 0 failed. ruff check/format clean.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011s8tBNnQTmb1q5nRAKwhvv
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
marm-console/tests/test_concept_build_concurrency.py (1)
15-57: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClean up global state after the test to ensure isolation.
The test populates the global
_launching_concept_buildsdictionary but only clears it at the beginning. This leaks up to 1,200 dummy jobs into subsequent tests, violating test isolation. As per path instructions for**/tests/**, tests must be properly isolated.Wrap the execution in a
try...finallyblock to ensure the dictionary is also cleared after the test runs.🔧 Proposed fix
def test_launching_concept_builds_survives_concurrent_insert_and_prune(): concepts_endpoint._launching_concept_builds.clear() - errors: list[Exception] = [] - - def insert_jobs(worker_id: int) -> None: - for i in range(300): - job_id = f"job-{worker_id}-{i}" - try: - with concepts_endpoint._launching_concept_builds_lock: - concepts_endpoint._launching_concept_builds[job_id] = ( - {"id": job_id, "status": "queued"}, - time.monotonic(), - ) - except Exception as exc: # noqa: BLE001 - capturing for the assertion - errors.append(exc) - - def prune_repeatedly() -> None: - for _ in range(300): - try: - concepts_endpoint._prune_launching_concept_builds() - except Exception as exc: # noqa: BLE001 - capturing for the assertion - errors.append(exc) - - def read_repeatedly() -> None: - for i in range(300): - try: - with concepts_endpoint._launching_concept_builds_lock: - concepts_endpoint._launching_concept_builds.get(f"job-0-{i}") - except Exception as exc: # noqa: BLE001 - capturing for the assertion - errors.append(exc) - - threads = ( - [threading.Thread(target=insert_jobs, args=(w,)) for w in range(4)] - + [threading.Thread(target=prune_repeatedly) for _ in range(4)] - + [threading.Thread(target=read_repeatedly) for _ in range(2)] - ) - for thread in threads: - thread.start() - for thread in threads: - thread.join() - - assert errors == [] + try: + errors: list[Exception] = [] + + def insert_jobs(worker_id: int) -> None: + for i in range(300): + job_id = f"job-{worker_id}-{i}" + try: + with concepts_endpoint._launching_concept_builds_lock: + concepts_endpoint._launching_concept_builds[job_id] = ( + {"id": job_id, "status": "queued"}, + time.monotonic(), + ) + except Exception as exc: # noqa: BLE001 - capturing for the assertion + errors.append(exc) + + def prune_repeatedly() -> None: + for _ in range(300): + try: + concepts_endpoint._prune_launching_concept_builds() + except Exception as exc: # noqa: BLE001 - capturing for the assertion + errors.append(exc) + + def read_repeatedly() -> None: + for i in range(300): + try: + with concepts_endpoint._launching_concept_builds_lock: + concepts_endpoint._launching_concept_builds.get(f"job-0-{i}") + except Exception as exc: # noqa: BLE001 - capturing for the assertion + errors.append(exc) + + threads = ( + [threading.Thread(target=insert_jobs, args=(w,)) for w in range(4)] + + [threading.Thread(target=prune_repeatedly) for _ in range(4)] + + [threading.Thread(target=read_repeatedly) for _ in range(2)] + ) + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + assert errors == [] + finally: + concepts_endpoint._launching_concept_builds.clear()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@marm-console/tests/test_concept_build_concurrency.py` around lines 15 - 57, Update test_launching_concept_builds_survives_concurrent_insert_and_prune to wrap the threaded test execution and assertion in a try...finally block, clearing concepts_endpoint._launching_concept_builds in the finally clause. Preserve the existing initial clear and all concurrency assertions while ensuring global state is cleaned up even when the test fails.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@marm-console/tests/test_concept_build_concurrency.py`:
- Around line 15-57: Update
test_launching_concept_builds_survives_concurrent_insert_and_prune to wrap the
threaded test execution and assertion in a try...finally block, clearing
concepts_endpoint._launching_concept_builds in the finally clause. Preserve the
existing initial clear and all concurrency assertions while ensuring global
state is cleaned up even when the test fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7d419878-4029-4fa2-adea-1084772d619b
📒 Files selected for processing (12)
marm-console/server/endpoints/compaction.pymarm-console/server/endpoints/concepts.pymarm-console/server/endpoints/logs.pymarm-console/server/endpoints/memory.pymarm-console/server/endpoints/overview.pymarm-console/server/endpoints/projects.pymarm-console/server/memory_store.pymarm-console/tests/test_concept_build_concurrency.pymarm-console/tests/test_memory_dashboard_gap_routes.pymarm-console/tests/test_memory_mutation_routes.pymarm-console/tests/test_overview_health.pymarm-console/tests/test_project_routes.py
🚧 Files skipped from review as they are similar to previous changes (5)
- marm-console/server/endpoints/compaction.py
- marm-console/server/endpoints/memory.py
- marm-console/server/endpoints/projects.py
- marm-console/server/endpoints/overview.py
- marm-console/server/endpoints/concepts.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/tests/**
⚙️ CodeRabbit configuration file
**/tests/**: Focus on tests that are flaky, non-isolated, incorrectly asserting behavior, or missing coverage for a changed high-risk path. Skip minor naming, comments, and layout preferences.
Files:
marm-console/tests/test_overview_health.pymarm-console/tests/test_concept_build_concurrency.pymarm-console/tests/test_memory_mutation_routes.pymarm-console/tests/test_project_routes.pymarm-console/tests/test_memory_dashboard_gap_routes.py
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Prioritize runtime correctness, async/concurrency safety, SQLite transaction safety, auth/rate-limit behavior, release-breaking packaging issues, and MCP protocol compatibility.
Files:
marm-console/tests/test_overview_health.pymarm-console/tests/test_concept_build_concurrency.pymarm-console/tests/test_memory_mutation_routes.pymarm-console/tests/test_project_routes.pymarm-console/server/memory_store.pymarm-console/server/endpoints/logs.pymarm-console/tests/test_memory_dashboard_gap_routes.py
🪛 ast-grep (0.44.1)
marm-console/tests/test_overview_health.py
[info] 17-17: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
marm-console/tests/test_memory_dashboard_gap_routes.py
[info] 439-439: use jsonify instead of json.dumps for JSON output
Context: json.dumps([f"m{i}"])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 OpenGrep (1.25.0)
marm-console/tests/test_memory_dashboard_gap_routes.py
[ERROR] 449-456: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 481-515: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
🔇 Additional comments (6)
marm-console/server/endpoints/logs.py (1)
58-84: LGTM!marm-console/server/memory_store.py (1)
412-422: LGTM!Also applies to: 425-431, 434-442
marm-console/tests/test_memory_dashboard_gap_routes.py (1)
3-3: LGTM!Also applies to: 46-51, 99-167, 432-538
marm-console/tests/test_memory_mutation_routes.py (1)
117-138: LGTM!Also applies to: 141-163
marm-console/tests/test_overview_health.py (1)
1-66: LGTM!marm-console/tests/test_project_routes.py (1)
136-155: LGTM!Also applies to: 158-171
- concepts.py: get_concept_build now copies the launch-record dict while still holding the lock, instead of returning a live reference the background build thread can keep mutating after the lock releases (torn read on status/error_code/finished_at). - test_concept_build_concurrency.py: wrap the stress test in try/finally and clear _launching_concept_builds under the lock both before and after, so it no longer leaves ~1,200 fake entries in shared module state for later tests in the same process to inherit. Full suite: 27 passed, 0 failed. ruff check/format clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011s8tBNnQTmb1q5nRAKwhvv
Both findings were confirmed real, pre-existing bugs in the original Knowledge.tsx (carried over verbatim by the split). Fixing here since this branch is the only place with easy access to the touched code right now, same reasoning as PR #99's follow-up fixes. - BuildAndDuplicates.tsx: isRunning now also covers the initial poll gap (jobId set but jobStatus not yet loaded), so the Close button can't appear before the job's real status is known. Added a close-effect that resets jobId/scope/scopeValue/confirmAll when the dialog closes, so reopening starts a fresh build instead of showing the previous job's finished state. - ExplorerTab.tsx: loadError is now checked before the stale-graph branch, so a failed seed/direction request shows the error state instead of silently re-rendering the previous successful graph. Verified: tsc --noEmit clean, npm run build succeeds, Playwright smoke pass against the live dev server confirmed no JS errors and the dialog reset behavior works end-to-end (open -> close -> reopen shows a fresh scope selector, not the prior job's status view). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011s8tBNnQTmb1q5nRAKwhvv
All 5 findings were confirmed real, pre-existing bugs in the original Memory.tsx (carried over verbatim by the split). Fixing here since this branch is the only place with easy access to the touched code right now, same reasoning as PRs #99/#100's follow-up fixes. - MemoriesTab.tsx: selectedIds now drops IDs no longer in the visible result set whenever search/filter results change, so bulk delete can't act on hidden stale selections. toggleAll's 'all selected' check now verifies every displayed ID is selected instead of just comparing set sizes. - MemoriesTab.tsx: clearing the context type field now sends null to the server instead of falling back to the memory's previous value. - NotebookAndCompactionTabs.tsx: name/project/platform are the notebook entry's identity key, so they're now locked once editing an existing entry -- editing them on save previously created a duplicate record instead of updating the original. - SessionsAndLogsTabs.tsx + NotebookAndCompactionTabs.tsx: session, log, notebook, and compaction mutations now all route their success/error outcomes through the single actionNotice state instead of independent per-mutation .error checks that could mask a newer result behind a stale one. - shared.tsx: deleteNotice now derives its base message from the actual deletedCount (including zero) and returns 'warning' whenever any requested ID was missing, instead of always reporting success. Verified: tsc --noEmit clean, npm run build succeeds, Playwright smoke pass against the live dev server hit all 5 tabs with zero JS errors. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011s8tBNnQTmb1q5nRAKwhvv
Pure refactor, no behavior changes, no route/path/method changes. Mirrors marm-mcp-server's own core/models.py + endpoints/*.py + router registration convention (confirmed with user over a lighter grouping). Extracted via exact index-based line-slicing -- all 15 pieces (including 4 non-contiguous ones) verified byte-identical to the original before any import cleanup.
Deliberate departure from a literal 1:1 mirror: /health, CORS, and the startup event stay in app.py rather than getting their own endpoints/system.py -- one 3-line endpoint with zero shared deps would be exactly the "tiny module with no real boundary" refactor-middle- ground.md warns against.
Two real bugs caught during implementation, both by tooling working as designed:
Validated: ruff check + ruff format --check clean, full suite 18 passed / 0 failed (matches baseline), and a live TestClient smoke pass confirmed all 8 domain routers plus /health are correctly mounted (no silent 404s from a missing include_router call).
Summary by CodeRabbit
/healthendpoint, and startup warm-up.