refactor(#288): architecture debt - extract health, rename list, update C4

[sshekhar563]

Resolves #288.

This PR addresses four architectural debt and maintainability items identified in the dev audit:

1. Extract Duplicated Health & Cleanup Logic

• Problem: Health audit and cleanup logic was copy-pasted between src/soul_protocol/mcp/server.py and src/soul_protocol/cli/main.py. The two implementations had already drifted (the CLI gained orphan-node removal and backup; MCP did not).
• Solution: Extracted shared logic to a new module src/soul_protocol/runtime/health.py.
  • Exposes dataclasses HealthReport and CleanupResult so callers can customize the formatting (JSON for MCP vs Rich panel for CLI).
  • Exposes audit_health(), plan_cleanup(), and execute_cleanup().
  • MCP Drift Fixed: The MCP cleanup tool now properly includes orphan-node pruning.

2. Solve CLI Builtin Shadowing (list)

• Problem: The CLI's list() command shadowed Python's builtin list inside src/soul_protocol/cli/main.py, forcing 14 ugly builtins.list(...) workarounds throughout the file.
• Solution: Renamed the Python function to list_cmd(). Kept the CLI registration name as @cli.command("list") so there is zero change to the user-facing CLI command name (soul list). Removed builtins import and replaced all workarounds with standard Python list(...).

3. Regenerate Stale C4 Model

• Problem: docs/c4/model.json was missing 9 subsystems added since June 2026.
• Solution: Regenerated the model to include: engine, spec, eval, optimize, cognitive, context, dream, eternal, and health, along with 10 new relationships connecting them. Updated the CLI component description to reference the command module split.

4. Document Soul Class Split Plan

• Problem: The Soul orchestrator is a 3,300+ line class that causes constant merge conflicts across feature branches. Splitting it requires a dedicated multi-PR effort.
• Solution: Added a clear TODO(#288) at the top of src/soul_protocol/runtime/soul.py outlining the recommended mixin split plan to keep future efforts aligned.

---

Verification

• Ran python -m pytest tests/test_cli/test_health_cleanup_repair.py -v (All 21 health/cleanup tests passed).
• Ran full test suite. (All 258 tests passed except the test_private_key_has_0600_permissions test, which is a pre-existing failure on Windows).
• Verified soul list, soul health, and soul cleanup work identically via the CLI.

[prakashUXtech]

This is the good kind of extraction. I checked the thing I was most worried about, which is whether pulling health and cleanup into a shared module quietly flattened the CLI down to the weaker MCP version, and it did not: orphan-node removal, --low-importance, and the .soul.bak backup all survive, every --no-* toggle is still wired, and the thresholds (dedup 0.8, importance <= 2, the "Scored " staleness check) are unchanged. You also correctly left the backup in the CLI caller rather than dragging it into the shared module, which is the right call. The list rename is spot on too: keeping the explicit @cli.command("list") means soul list is unchanged for users, and that's the detail most people miss. All 14 builtins.list(...) workarounds and the import builtins are gone.

Deferring the Soul split with a concrete TODO(#288) is the right judgment. That one shouldn't be a drive-by.

Two blockers:

1. MCP soul_cleanup gained a destructive capability without the CLI's guard. mcp/server.py:1574 hardcodes orphan_nodes=True, so graph entities can now be permanently pruned through an LLM-invoked tool. On the CLI that same operation is deliberately paired with backup_soul_file() before the export, but the MCP path just marks the soul modified and saves on shutdown. No backup, no opt-out, no dry-run. The drift you're fixing went the other way on purpose: the CLI author added the backup because orphan pruning is irreversible. Either add backup_soul_file to the MCP path or expose orphan_nodes: bool = False so the caller opts in. Roughly five lines.

2. This collides head-on with your own open #299. That PR touches the same files and the same lines, adding the public accessors and replacing the private-attribute access in the CLI and MCP. This PR moves those exact blocks into runtime/health.py keeping the private access verbatim, so if #299 lands first, health.py quietly reintroduces the coupling #299 was written to delete, and whichever merges second will conflict on those hunks. Let's pick an order: I'd land #299 first, then rebase this onto its public API. Worth deciding before either merges.

Also worth fixing:

3. The new health filter can swallow real issues. cli/main.py:3422-3425 string-matches on "duplicate" and "orphan" to un-flatten the merged issues list. Skill ids are agent-learned and user-controllable, so a skill named something like orphan-cleanup with negative XP produces an issue string containing "orphan", gets filtered out, and isn't counted in issues_found. The panel can then print "No issues found, soul is healthy" while a real problem exists. Dev didn't have this hazard because it iterated separate bond_issues and skill_issues lists. Keeping those as structured fields on HealthReport (with issues as a derived property) fixes it and removes the fragile matching.

4. Two of the new C4 relationships aren't real. memory-tiers -> engine and context -> engine: nothing under runtime/memory/ imports soul_protocol.engine (the actual importers are cli/journal.py, cli/org.py, and spike/memory_journal.py), and runtime/context/store.py uses sqlite3 directly plus spec.context.models, never engine/. This matters more than a doc nit because loom reads model.json for blast-radius and boundary rules, so a wrong edge turns "returns nothing" into "returns something wrong", which is the worse failure. Suggest cli -> engine and context -> spec instead. The nine new components all check out. Minor: the note says "Regenerated" but the diff is clearly hand-authored, which is fine (and correctly keeps authored: true), just word it that way.

5. The MCP cleanup response renamed removed to total_removed. Any external MCP consumer reading data["removed"] now gets a KeyError. Nothing in-repo depends on it, but MCP tools get called by arbitrary agents, so either keep removed as an alias or call the rename out in the PR body.

6. No tests import the new module. Coverage is entirely indirect through the CLI end-to-end tests (which is what gave me confidence the CLI didn't regress) and four key-presence MCP tests. That MCP shape-only assertion is exactly why the removed rename went green. For a pure refactor this is the main risk, since a green suite proves very little about whether the extraction preserved semantics. A tests/test_runtime/test_health.py covering audit_health field by field, plan_cleanup per flag, and execute_cleanup's return count would close it. There's also no orphan-node test on either side.

Smaller: execute_cleanup does removed += 1 unconditionally while remove() returns a bool, and dedup plus low-importance can select the same id, so the count can overreport (copied from dev, so not a regression, but this is the natural place to fix it). The Updated: (#288) header lines are missing on cli/main.py and mcp/server.py. And lint is red only because the new file needs formatting: uv run ruff format src/soul_protocol/runtime/health.py.

The core of this refactor is sound. Everything above is at the edges.

[prakashUXtech]

Great response to the last round: five of the six findings are fixed, each with a regression test. The MCP soul_cleanup is now opt-in (orphan_nodes=False, dry-run default) with a best-effort backup before it prunes; the health issues are structured fields, so the orphan-cleanup skill-name hazard is gone (and there's a test using that exact adversarial name); the two fabricated C4 edges are replaced with real ones (cli -> engine, context -> spec); removed is kept as an alias alongside total_removed; and there's a real tests/test_runtime/test_health.py. Nicely done.

The one remaining item isn't a code bug, it's sequencing with #299. runtime/health.py still reaches through private internals (about 23 accesses) because it didn't adopt #299's public API, and those methods don't exist on dev yet since #299 introduces them. Both PRs edit the same regions of cli/main.py, mcp/server.py, and soul.py, so whichever merges second will conflict.

The plan I'd suggest: land #299 first (it's approved and ready), then rebase this so health.py calls the public API (episodic_entries(), graph_entities(), graph_remove_entity(), Soul.memory, eval_history). That drops the 23 private accesses to near zero and makes this PR genuinely debt-reducing rather than relocating the coupling. Once #299 is on dev and you've rebased, ping me and I'll re-review and approve. I'm holding the formal approve only on that rebase; the code itself is in good shape.