From 9a6fd8066b7e7c3064012dd66d7f55994995a52c Mon Sep 17 00:00:00 2001 From: Ed_ Date: Wed, 1 Jul 2026 18:41:18 -0400 Subject: [PATCH] docs(spec): MMA quarantine + RAG test decoupling design --- ...a-quarantine-rag-test-decoupling-design.md | 192 ++++++++++++++++++ 1 file changed, 192 insertions(+) create mode 100644 docs/superpowers/specs/2026-07-01-mma-quarantine-rag-test-decoupling-design.md diff --git a/docs/superpowers/specs/2026-07-01-mma-quarantine-rag-test-decoupling-design.md b/docs/superpowers/specs/2026-07-01-mma-quarantine-rag-test-decoupling-design.md new file mode 100644 index 00000000..132a8fe7 --- /dev/null +++ b/docs/superpowers/specs/2026-07-01-mma-quarantine-rag-test-decoupling-design.md @@ -0,0 +1,192 @@ +# Design: MMA Quarantine + RAG Test Decoupling + +**Date:** 2026-07-01 +**Status:** Draft (pending user review) +**Scope:** Two surgical interventions. (1) Quarantine the MMA automation engine behind a config flag so it stops consuming test-suite time and stops breaking when adjacent code changes. (2) Decouple the RAG tests from the live_gui subprocess + chromadb file locks so the default test batch stops bleeding on RAG. + +**Out of scope:** The discussion/session system redesign (owned by the nagent research track). Full removal of MMA code (deferred to a follow-up track if quarantine maintenance becomes painful). Any change to the RAG algorithm itself (`index_file`, `search`, chunking — unchanged). The `conductor/` track system (the `conductor_tech_lead.py` / `project_manager.py` / `dag_engine.py` shared-types layer is preserved as load-bearing). + +## Context + +### Why this work (grounded in git history, not track docs) + +The last 10 days of git history (2026-06-20 through 2026-06-30) show two subsystems consuming disproportionate debugging time: + +**RAG test debugging churn (2026-06-27):** ~10 commits chasing RAG test failures — `_get_chromadb()` NameError in dim check, file-lock dim-check failures, silent `index_file` no-ops on missing files, session-scoped subprocess pollution, hotpatched state instead of project-switch. Two "ADDENDUM" reports because the first root-cause diagnosis was wrong. The commits are fighting the *environment* (chromadb file locks under the live_gui subprocess, CWD drift across the spawn boundary, lazy global teardown/rebuild), not the RAG algorithm. + +**MMA concurrent tracks sim fix (2026-06-27):** `fix_mma_concurrent_tracks_sim_20260627` — 5 fixes to `mock_concurrent_mma.py` (session_id fallback removal, epic branch catch-all, sprint routing by prompt content) plus a `refresh_from_project` task that was overwriting `self.tracks`. The mock itself is brittle; the production engine is brittle in the same shape. + +The user's direction: sunset MMA constructively (quarantine, not delete — full removal is a follow-up if quarantine maintenance hurts), keep RAG but make the tests sane. + +### The shared-types finding (why this is quarantine, not removal) + +`src/mma.py` is a shared types module, not just the MMA engine. Non-MMA code depends on it: +- `thinking_parser.py` → `ThinkingSegment` (thinking-trace parsing, used in every AI turn — NOT an MMA feature) +- `project_manager.py` → `TrackState`, `EMPTY_TRACK_STATE` (the conductor track system — persists `conductor/tracks//state.toml`) +- `models.py` → `TrackMetadata` (re-exported as legacy `Metadata` alias) +- `dag_engine.py` → `Ticket` +- `conductor_tech_lead.py` → `Ticket`, `TrackDAG` + +`dag_engine.py` is shared between the MMA loop AND `conductor_tech_lead.py` (the Tier 2 tech lead system). Full removal would require migrating `ThinkingSegment` and `TrackState`/`TrackMetadata` out of `mma.py` first — a mutation of the shared-types module that risks the conductor track system. That work is deferred to a follow-up track. + +### The RAG fragility root cause + +The RAG algorithm is ~50 lines (`index_file` + `search` + chunking). The other ~250 lines of `rag_engine.py` are defensive scaffolding for the test/subprocess environment: +- `_validate_collection_dim_result` uses `shutil.rmtree(ignore_errors=True)` to survive Windows file locks held by the live_gui subprocess +- `index_file` has a CWD-fallback band-aid for path resolution drift across the spawn boundary +- `_sync_rag_engine` is called from 6 sites in `app_controller.py` (files change, project switch, session reset, RAG toggle) — 6 race surfaces +- Lazy module globals (`_CHROMADB`, `_GOOGLE_GENAI`, `_SENTENCE_TRANSFORMERS`) tear down and rebuild per-test with no guarantee of state + +The RAG *tests* are testing "does RAG survive the live_gui subprocess lifecycle + chromadb file locks + CWD drift + lazy global teardown/rebuild" — not "does RAG retrieve relevant chunks." That's the bleed. + +## Section 1: MMA Quarantine + +### Mechanism + +Config flag `mma.enabled`, default `false`, in `[ai_settings.toml]`. Per `conductor/code_styleguides/feature_flags.md` §2: this is a persistent preference (off by default, not recoverable by a single regenerate command), so config flag + GUI checkbox is the correct pattern — not file presence, not env-var-only. + +### What the flag gates + +#### 1.1 `app_controller.py` (~20 sites) + +State fields initialized to empty/zero-init when `mma.enabled == false`: +- `self.engines: Dict[str, ConductorEngine] = {}` (stays empty) +- `self.mma_streams: Dict[str, str] = {}` (stays empty) +- `self.mma_step_mode: bool = False` (stays False) +- `self.mma_tier_usage: Dict[str, Metadata] = {...}` (stays zero-init) +- `self.tracks: list[Metadata] = []` (stays empty — note: this is the MMA track list, NOT the conductor track system; `project_manager.get_all_tracks` is still callable for the conductor UI if needed) + +Engine methods return early when the flag is off: +- `start_mma` / engine-start paths — return early, no `ConductorEngine` instantiation +- `approve_step` / `approve_spawn` — return early, no-op +- abort / reset paths — clear the (already empty) state + +The `multi_agent_conductor` import becomes lazy: imported inside the gated methods only, gated on the flag. When the flag is off, the module is never imported at runtime (reduces import-graph weight). + +The `rag_engine` sync paths are untouched — RAG is a separate subsystem (Section 2). + +#### 1.2 `gui_2.py` (12 render functions + dashboard window + modals) + +Every `render_mma_*` function and `render_task_dag_panel` checks `if not app.controller.mma_enabled: return` at the top: +- `render_mma_dashboard` (gui_2.py:6610) +- `render_mma_modals` (gui_2.py:6658) +- `render_mma_track_summary` (gui_2.py:6757) +- `render_mma_epic_planner` (gui_2.py:6800) +- `render_mma_conductor_setup` (gui_2.py:6818) +- `render_mma_track_browser` (gui_2.py:6837) +- `render_mma_global_controls` (gui_2.py:6884) +- `render_mma_usage_section` (gui_2.py:6924) +- `render_mma_ticket_editor` (gui_2.py:7003) +- `render_mma_agent_streams` (gui_2.py:7043) +- `render_task_dag_panel` (gui_2.py:7351) +- `render_mma_focus_selector` (gui_2.py:7570) + +The MMA Dashboard window is not registered in the panel registry when the flag is off (gui_2.py:1995 `_render_window_if_open("MMA Dashboard", ...)` — gated on flag). The approval modals (MMA Step Approval, MMA Spawn Approval) no-op. The ~600 lines of render code stay in-tree but are dead at runtime. + +#### 1.3 `multi_agent_conductor.py` + +Kept in-tree. Imported lazily only inside the gated `app_controller` methods. `ConductorEngine` / `WorkerPool` never instantiate when the flag is off. + +#### 1.4 What stays active (shared types, NOT gated) + +- `mma.py` — `Ticket`, `Track`, `TrackState`, `TrackMetadata`, `WorkerContext`, `ThinkingSegment` — load-bearing for non-MMA code +- `dag_engine.py` — `TrackDAG`, `ExecutionEngine` — used by `conductor_tech_lead.py` +- `mma_prompts.py` — used by `ai_client.py`, `conductor_tech_lead.py`, `orchestrator_pm.py` (verify whether these usages are MMA-specific or general during implementation; if MMA-specific, gate; if general, leave) +- `mma.py` / `dag_engine.py` / `mma_prompts.py` imports in non-MMA consumers remain unchanged + +#### 1.5 GUI surface + +A single `[ ] Enable MMA (deprecated, quarantined)` checkbox in AI Settings. The full MMA dashboard is hidden when the flag is off. The checkbox is the only MMA UI surface. Per `feature_flags.md` §2: the GUI checkbox is a projection of the config file; the config file is the source of truth. + +#### 1.6 Tests (env-gated, opt-in) + +MMA tests gated behind `SLOP_MMA_TESTS=1` env var via `@pytest.mark.skipif`. Skip reason documents: "MMA is quarantined; run with `SLOP_MMA_TESTS=1` to enable." This is the *test* gate — separate from the *runtime* config flag, per `feature_flags.md` §6 (layered flags: file presence / config for runtime, env var for test opt-in). + +Affected test files (the `test_mma_*`, `test_concurrent_*`, `test_conductor_*`, `test_dag_*`, `test_parallel_*`, `test_worker_*`, `test_visual_mma*`, `test_*sim*mma*` set): +- `test_mma_agent_focus_phase1.py`, `test_mma_agent_focus_phase3.py` +- `test_mma_approval_indicators.py`, `test_mma_concurrent_tracks_sim.py`, `test_mma_concurrent_tracks_stress_sim.py` +- `test_mma_dashboard_refresh.py`, `test_mma_dashboard_streams.py`, `test_mma_models.py`, `test_mma_node_editor.py` +- `test_mma_orchestration_gui.py`, `test_mma_prompts.py`, `test_mma_skeleton.py`, `test_mma_step_mode_sim.py` +- `test_mma_ticket_actions.py`, `test_mma_tier_usage_reset_fix.py`, `test_mma_usage_stats.py` +- `test_mma_concurrent_tracks_sim.py`, `test_mma_concurrent_tracks_stress_sim.py` +- `test_reset_session_clears_mma_and_rag.py` (split: MMA portion gated, RAG portion stays — see Section 2) +- `test_visual_mma.py`, `test_visual_sim_mma_v2.py` +- `mock_concurrent_mma.py` (test helper, not a test file — kept as-is for opt-in runs) +- `test_conductor_abort_event.py`, `test_conductor_api_hook_integration.py`, `test_conductor_engine_abort.py`, `test_conductor_engine_v2.py`, `test_conductor_tech_lead.py` +- `test_dag_engine.py`, `test_gui_dag_beads.py`, `test_perf_dag.py`, `test_task_dag_popout_sim.py` +- `test_parallel_execution.py`, `test_run_worker_lifecycle_abort.py` + +Note: `test_conductor_tech_lead.py` and `test_dag_engine.py` may test the shared-types layer (not the MMA engine). During implementation, classify each: if it tests the shared `dag_engine`/`conductor_tech_lead` layer (not the MMA engine), it stays in the default batch. If it tests the MMA engine specifically, it's gated. The classifier: does the test import or instantiate `multi_agent_conductor.ConductorEngine` / `WorkerPool`? If yes → gated. If no → stays. + +## Section 2: RAG Test Decoupling + +### Mechanism + +Three-tier test classification. The RAG algorithm is unchanged. The mock provider already exists (`rag_engine.py`: `provider == 'mock'` short-circuits chromadb). The tests are reclassified, not rewritten from scratch. + +### Tier 1 — Unit tests (default batch, no chromadb, no subprocess) + +Test the 50-line algorithm in isolation against the mock provider: +- `test_rag_chunk.py` — `RAGChunk` dataclass (already isolated; stays) +- `test_rag_engine.py` — rewrite to use mock provider exclusively; test `index_file` no-op behavior on mock, `search` returns `[]` on mock, chunking logic (`_chunk_text`, `_chunk_code_result`), `is_empty` semantics, `RAGChunk.from_dict`/`to_dict` round-trip +- `test_rag_engine_result.py` — Result wrapping (already isolated; stays) +- `test_rag_sync_none_error.py` — sync error handling (mock the engine, not chromadb) + +### Tier 2 — Controller lifecycle tests (default batch, mock RAGEngine, no chromadb) + +Test `app_controller.py`'s RAG lifecycle wiring using a mock/stub `RAGEngine`: +- `test_rag_engine_ready_status_bug.py` — mock the `rag_engine` on `AppController`, test the ready-status state machine +- `test_rag_gui_presence.py` — test the GUI panel renders when RAG is enabled/disabled (no real engine) +- `test_sync_rag_engine_coalescing.py` — test `_sync_rag_engine` coalescing logic with a mock engine (controller logic, not RAG logic) +- `test_reset_session_clears_mma_and_rag.py` — RAG portion stays (test reset clears the RAG state fields with a mock engine); MMA portion gated per Section 1.6 + +### Tier 3 — Integration tests (opt-in, env-gated, real chromadb / live_gui) + +Gated behind `SLOP_RAG_INTEGRATION=1` env var via `@pytest.mark.skipif`. Skip reason: "Integration test requires real chromadb + live_gui subprocess; run with `SLOP_RAG_INTEGRATION=1` to enable." These are the tests that kept breaking — now opt-in, not default batch: +- `test_rag_phase4_final_verify.py` — the 3-ADDENDUM fragile one +- `test_rag_phase4_stress.py` — stress test +- `test_rag_visual_sim.py` — live_gui visual sim +- `test_rag_integration.py` — full integration + +### What this kills + +The recurring bleed. The default test batch no longer touches chromadb file locks, the live_gui subprocess RAG state, or CWD drift. When adjacent code changes, Tier 1+2 verify RAG logic + controller wiring in isolation. Tier 3 only runs on explicit opt-in (e.g., before a release, or when actively working on RAG). + +### What this preserves + +The RAG feature itself. `RAGEngine` is unchanged. The mock provider already exists. The integration tests still exist — just not in the default batch. + +## Verification + +### MMA quarantine +- `mma.enabled = false` (default): MMA dashboard does not render; engine methods no-op; `multi_agent_conductor` not imported at runtime; MMA tests skipped (not failed) +- `mma.enabled = true`: MMA dashboard renders; engine starts; MMA tests run when `SLOP_MMA_TESTS=1` +- No regression in non-MMA code (thinking_parser, project_manager, models, conductor_tech_lead — shared types intact) + +### RAG test decoupling +- Default batch: Tier 1+2 run in milliseconds, no chromadb, no subprocess, no file locks +- `SLOP_RAG_INTEGRATION=1`: Tier 3 runs (the previously-fragile tests) +- RAG feature functional end-to-end when `rag.enabled = true` in config (unchanged) + +## Risks + +- **Lazy import correctness:** the `multi_agent_conductor` lazy import inside gated methods must not introduce a circular import or a startup-time import when the flag is off. Verify via `scripts/audit_main_thread_imports.py` after implementation. +- **Shared-types boundary:** `mma.py` / `dag_engine.py` / `mma_prompts.py` must remain importable by non-MMA consumers. The flag gates the *engine*, not the *types*. If `mma_prompts.py` usages in `ai_client.py` / `conductor_tech_lead.py` / `orchestrator_pm.py` are MMA-specific (e.g., prompt templates only used by the engine), gate them; if general, leave. Classify during implementation. +- **Test classifier drift:** the MMA test classifier ("does it import `ConductorEngine` / `WorkerPool`?") must be applied consistently. A test that tests shared `dag_engine` types but not the engine stays in the default batch. +- **Quarantine maintenance cost:** if the quarantined code rots (imports drift, shared types change underneath it), follow-up full removal (Option A) becomes necessary. The quarantine is the lower-risk path *now*; it's not a permanent commitment. + +## Out of Scope + +- The discussion/session system redesign (nagent research track) +- Full removal of MMA code (follow-up track if quarantine maintenance hurts) +- RAG algorithm changes (`index_file`, `search`, chunking — unchanged) +- The `conductor/` track system (`conductor_tech_lead.py` / `project_manager.py` / `dag_engine.py` shared-types layer preserved) +- Migrating `ThinkingSegment` / `TrackState` / `TrackMetadata` out of `mma.py` (follow-up to full removal) + +## See Also + +- `conductor/code_styleguides/feature_flags.md` — the config-flag-vs-file-presence decision tree (§2: persistent preference → config flag + GUI checkbox) +- `docs/guide_mma.md` — the MMA engine architecture (the thing being quarantined) +- `docs/guide_rag.md` — the RAG subsystem architecture (unchanged) +- `conductor/tracks/fix_mma_concurrent_tracks_sim_20260627/` — the recent MMA brittleness +- Git history 2026-06-27 — the RAG test debugging churn (10 commits, 2 ADDENDUM reports) \ No newline at end of file