ed/manual_slop
Private
Public Access
0
0
Fork 0
Code Activity

conductor(state): fix_mma_concurrent_tracks_sim_20260627 SHIPPED (with stress test fix)

Browse Source 
Track complete. All 7 VCs pass. Both tests now pass:
- test_mma_concurrent_tracks_execution: PASS (5 runs verified)
- test_mma_concurrent_tracks_stress: PASS (3 runs verified)

3 fixes shipped in this track:
- e9919059: TrackMetadata import (production NameError)
- 913aa48c: Mock sprint routing (session_id-based was fragile)
- fad1755b: Mock epic catch-all (literal-substring was fragile)

Parent branch tier2/post_module_taxonomy_de_cruft_20260627 is now
ready for merge after this fix track is reviewed.

OUTSTANDING_MMA_TEST_FAILURES_20260627.md updated to RESOLVED
status for all 5 stacked regressions. TRACK_COMPLETION report
updated to document all 3 fixes and the verification results.
This commit is contained in:
ed
2026-06-27 15:00:59 -04:00
parent fad1755b7d
commit 65928055fa
3 changed files with 104 additions and 60 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/reports/OUTSTANDING_MMA_TEST_FAILURES_20260627.md
+13 -1
View File
@@ -131,4 +131,16 @@ If the user wants me to **continue in this session**, I can:
4. Verify the test passes
5. Commit the fix
Per user direction, no sweeping under the rug — this needs a real fix.
Per user direction, no sweeping under the rug — this needs a real fix.
### 6. ✅ **RESOLVED** — Mock bug: epic branch only matches one literal prompt
**Date:** 2026-06-27 (discovered after the fix_mma_concurrent_tracks_sim_20260627 track SHIPPED)
The stress test (`tests/test_mma_concurrent_tracks_stress_sim.py::test_mma_concurrent_tracks_stress`) uses `mma_epic_input='STRESS TEST: TRACK A AND TRACK B'`, which the mock's epic branch did NOT match (it only matched `'PATH: Epic Initialization'`). The stress prompt fell to the Default branch which returns text (not JSON), and the production's `orchestrator_pm.generate_tracks` failed to parse it, returning 0 tracks.
**Root cause:** The mock's epic branch was a literal-substring check for a single test-specific prompt. It was not robust to other test prompts.
**Status:** ✅ **FIXED** in commit `fad1755b` (restructured routing so sprint and worker are checked first, and any non-empty prompt that doesn't match those patterns is treated as an epic request returning 2 tracks).
**Verification:** 3 consecutive PASS runs of both `test_mma_concurrent_tracks_execution` AND `test_mma_concurrent_tracks_stress` (13.94s, 14.81s, 14.13s).
docs/reports/TRACK_COMPLETION_fix_mma_concurrent_tracks_sim_20260627.md
+63 -39
View File
@@ -1,44 +1,55 @@
# Track Completion: fix_mma_concurrent_tracks_sim_20260627
**Date:** 2026-06-27
**Date:** 2026-06-27 (initial SHIP: 2026-06-27; stress test fix added 2026-06-27)
**Branch:** `tier2/post_module_taxonomy_de_cruft_20260627`
**Status:** SHIPPED. All 7 VCs pass. The tier-3-live_gui::test_mma_concurrent_tracks_sim::test_mma_concurrent_tracks_execution test now passes in the batched test suite (5 consecutive PASS runs).
**Status:** SHIPPED. All 7 VCs pass. Both `test_mma_concurrent_tracks_execution` AND `test_mma_concurrent_tracks_stress` now pass in the batched test suite (3 consecutive PASS runs).
---
## TL;DR
The 1 remaining tier-3-live_gui failure (`test_mma_concurrent_tracks_execution`) was caused by **two stacked bugs**, not just one:
The 1 remaining tier-3-live_gui failure (`test_mma_concurrent_tracks_execution`) was caused by **THREE stacked bugs**, not just two. A follow-up test run revealed a 3rd bug in the mock that caused `test_mma_concurrent_tracks_stress` to fail (0 tracks created).
1. **Production bug (FIXED in `e9919059`):** `src/app_controller.py:_start_track_logic_result` used `models.Metadata(...)` (line 4830) but the `from src import models` import was removed in commit `ee763eea` (the de-cruft migration). The existing EXCEPT block caught only 7 exception types (not NameError), so the NameError propagated up, the io_pool worker died, and the for loop in `_cb_accept_tracks._bg_task` never reached track-b. Track-a was never appended to `self.tracks`, so the test poll for `tracks >= 2` timed out.
### Bug 1: Production NameError (FIXED in `e9919059`)
2. **Mock bug (FIXED in `913aa48c`):** The session_id-based routing added in commit `635ca552` had two sub-bugs:
- `call_n` literal matching (`== 2`, `== 3`) is fragile to test ordering: the file-based counter persists across tests in the same session, so `call_n != 2` for the 1st sprint if a prior test ran.
- `session_id="mock-sprint-A"` means "this is a follow-up call after the 1st sprint returned mock-sprint-A", so the response should be **sprint-B** (2nd track tickets), not sprint-A. The prior code routed this to sprint-A, which means track-b's worker has stream id `ticket-A-1` (not `ticket-B-1`) and the test's `ticket-B-1` poll never finds it.
`src/app_controller.py:_start_track_logic_result` used `models.Metadata(...)` (line 4830) but the `from src import models` import was removed in commit `ee763eea` (the de-cruft migration). The existing EXCEPT block caught only 7 exception types (not NameError), so the NameError propagated up, the io_pool worker died, and the for loop in `_cb_accept_tracks._bg_task` never reached track-b. Track-a was never appended to `self.tracks`, so the test poll for `tracks >= 2` timed out.
**Fixes:**
- Add `TrackMetadata` to the `from src.mma import` line in `src/app_controller.py`
- Change `models.Metadata(...)` to `TrackMetadata(...)`
- Replace the session_id-based sprint routing in `tests/mock_concurrent_mma.py` with prompt-content-based routing
**Fix:** Add `TrackMetadata` to the `from src.mma import` line; change `models.Metadata(...)` to `TrackMetadata(...)`.
### Bug 2: Mock sprint routing fragile (FIXED in `913aa48c`)
The session_id-based routing added in commit `635ca552` had two sub-bugs:
- `call_n` literal matching (`== 2`, `== 3`) is fragile to test ordering: the file-based counter persists across tests in the same session, so `call_n != 2` for the 1st sprint if a prior test ran.
- `session_id="mock-sprint-A"` means "this is a follow-up call after the 1st sprint returned mock-sprint-A", so the response should be **sprint-B** (2nd track tickets), not sprint-A. The prior code routed this to sprint-A, which means track-b's worker has stream id `ticket-A-1` (not `ticket-B-1`) and the test's `ticket-B-1` poll never finds it.
**Fix:** Replace the session_id-based sprint routing with prompt-content-based routing.
### Bug 3: Mock epic branch only matches one literal prompt (FIXED in `fad1755b`)
The stress test uses `mma_epic_input='STRESS TEST: TRACK A AND TRACK B'`, which the mock's epic branch did NOT match (it only matched `'PATH: Epic Initialization'` literal substring). The stress prompt fell to the Default branch which returns text (not JSON), and the production's `orchestrator_pm.generate_tracks` failed to parse it, returning 0 tracks. The test polled for proposed_tracks (60s timeout, never broke), clicked accept (no proposed_tracks to process), then asserted `tracks >= 2` and found 0.
**Root cause:** The mock's epic branch was a literal-substring check for a single test-specific prompt. It was not robust to other test prompts.
**Fix:** Restructure routing so that sprint and worker are checked first (more specific patterns), and ANY non-empty prompt that does not match those patterns is treated as an epic request (returns 2 tracks). Empty prompts fall to the Default branch.
---
## Commits Applied (4 atomic commits)
## Commits Applied (7 atomic commits)
| SHA | Type | Description |
|---|---|---|
| `ee185758` | conductor(track) | Initialize fix_mma_concurrent_tracks_sim_20260627 (spec, plan, metadata, state) |
| `75fdebb0` | chore(diag) | Add stderr instrumentation to _start_track_logic_result (interim, removed) |
| `d046394a` | chore(diag) | Add file-based diag instrumentation for MMA tracks (interim, removed) |
| `e9919059` | fix(mma_concurrent) | Import TrackMetadata directly to fix NameError |
| `e9919059` | fix(mma_concurrent) | Import TrackMetadata directly to fix NameError (Bug 1) |
| `23862d35` | chore(cleanup) | Remove all diagnostic instrumentation from app_controller |
| `913aa48c` | fix(mock_concurrent_mma) | Route sprints on prompt content not session_id |
Plus per-task plan-update commits per workflow.md Per-Task Commit Protocol.
| `913aa48c` | fix(mock_concurrent_mma) | Route sprints on prompt content not session_id (Bug 2) |
| `7c98a2dc` | conductor(state) | Initial SHIPPED + TRACK_COMPLETION + OUTSTANDING update |
| `fad1755b` | fix(mock_concurrent_mma) | Make epic branch a catch-all for non-empty prompts (Bug 3) |
---
## Root Cause Analysis (the 4 stacked regressions from OUTSTANDING_MMA_TEST_FAILURES_20260627.md)
## Root Cause Analysis (the 5 stacked regressions from OUTSTANDING_MMA_TEST_FAILURES_20260627.md)
### 1. `flat_config()` return type change (PRODUCTION BUG — FIXED in 635ca552)
@@ -59,16 +70,29 @@ The `gemini_cli_adapter` reuses the session_id from the previous call via `--res
**Fix in 635ca552:** Added session_id-based routing with a file-based call counter.
**Fix in 913aa48c:** Replaced session_id-based routing with prompt-content-based routing (the original pre-`635ca552` design, which is more robust).
### 4. ⚠️ The actual production bug (FIXED in e9919059)
### 4. ✅ **RESOLVED** (e9919059) — Production bug: NameError on `models.Metadata` call site
The EXCEPT block in `_start_track_logic_result` catches only 7 exception types (`OSError, IOError, ValueError, TypeError, KeyError, AttributeError, RuntimeError`). A `NameError` (caused by the removed `from src import models` import) propagated up, killing the io_pool worker, and the for loop in `_cb_accept_tracks._bg_task` never reached track-b.
After all 3 prior fixes in commit `635ca552`, only 1 sprint-ticket call was observed (for track-a). The for loop in `_cb_accept_tracks._bg_task` was reached but track-a's `_start_track_logic` raised a `NameError` that was NOT caught by the EXCEPT block (which only catches 7 specific exception types). The io_pool worker died, the for loop never reached track-b.
**Root cause:** The de-cruft migration in commit `ee763eea` removed `from src import models` from `src/app_controller.py` but did not update the call site `models.Metadata(...)` (line 4830).
**Root cause:** The de-cruft migration in commit `ee763eea` removed `from src import models` from `src/app_controller.py` but did not update the call site `models.Metadata(...)` at line 4830.
**Fix in e9919059:**
- Add `TrackMetadata` to the `from src.mma import` line
- Change `models.Metadata(...)` to `TrackMetadata(...)`
- Restore the EXCEPT block to the original 7 types (narrowing the BaseException diagnostic back)
**Fix:** Add `TrackMetadata` to the `from src.mma import` line; change `models.Metadata(...)` to `TrackMetadata(...)`.
### 5. ✅ **RESOLVED** (913aa48c) — Mock bug: session_id-based routing for sprints is fragile
The session_id-based routing added in commit `635ca552` had two sub-bugs:
- `call_n` literal matching (`== 2`, `== 3`) is fragile to test ordering: the file-based counter persists across tests in the same session, so `call_n != 2` for the 1st sprint if a prior test ran.
- `session_id="mock-sprint-A"` means "this is a follow-up call after the 1st sprint returned mock-sprint-A", so the response should be sprint-B (2nd track tickets), not sprint-A. The prior code routed this to sprint-A, causing track-b's worker to have stream id `ticket-A-1` (not `ticket-B-1`).
**Fix:** Replaced session_id-based sprint routing with prompt-content-based routing; the original pre-`635ca552` design.
### 6. ✅ **RESOLVED** (fad1755b) — Mock bug: epic branch only matches one literal prompt
**Discovered after the initial SHIP** when the user ran the batched test suite and `test_mma_concurrent_tracks_stress_sim` failed.
**Root cause:** The mock's epic branch was a literal-substring check for `'PATH: Epic Initialization'`. The stress test uses `'STRESS TEST: TRACK A AND TRACK B'` which didn't match, so it fell to the Default branch (returns text, not JSON). The production's `orchestrator_pm.generate_tracks` failed to parse, returned `[]`.
**Fix:** Restructured mock routing so that sprint and worker are checked first (more specific patterns), and ANY non-empty prompt that does not match those patterns is treated as an epic request (returns 2 tracks). Empty prompts fall to the Default branch.
---
@@ -102,21 +126,21 @@ Both tracks are now created successfully.
The instrumentation was removed in commit `23862d35` (per `edit_workflow.md` §9 "No Diagnostic Noise in Production Code"). 38 lines of `try/except` + `with open(...)` diag blocks were deleted.
The stress test fix in `fad1755b` did NOT require new diagnostic instrumentation — the root cause was identified by code reading (the mock's epic branch was a literal-substring check, and the stress test uses a different prompt).
---
## Verification Results
### Test Stability (5 consecutive runs)
### Test Stability (3 consecutive runs, BOTH tests)
| Run | Result | Time |
|---|---|---|
| 1 | PASS | 7.97s |
| 2 | PASS | 7.49s |
| 3 | PASS | 8.45s |
| 4 | PASS | 8.02s |
| 5 | PASS | 7.54s |
| 1 | PASS (both) | 13.94s |
| 2 | PASS (both) | 14.81s |
| 3 | PASS (both) | 14.13s |
**Flakiness: 0%** (was previously flaky: 1 of 4 runs failed with the final stability check, 1 of 4 failed with "Worker from Track B never appeared")
**Flakiness: 0%** (was previously 100% for stress test, ~25% for execution test)
### Audit Scripts
@@ -134,7 +158,7 @@ The instrumentation was removed in commit `23862d35` (per `edit_workflow.md` §9
| `tests/test_app_controller_result.py` (excluding `test_app_controller_does_not_use_broad_except`) | 33 passed, 1 deselected (pre-existing) |
| `tests/test_conductor_tech_lead.py` | All passed (9 tests) |
**Pre-existing failure (NOT introduced by this track):** `tests/test_app_controller_result.py::test_app_controller_does_not_use_broad_except` fails because `src/app_controller.py` has 8 `INTERNAL_BROAD_CATCH` sites (the except blocks catching 7 exception types each). This is a pre-existing failure unrelated to this track. Confirmed by running the test on the parent commit (e9919059's parent, d046394a) — the test was already failing then.
**Pre-existing failure (NOT introduced by this track):** `tests/test_app_controller_result.py::test_app_controller_does_not_use_broad_except` fails because `src/app_controller.py` has 8 `INTERNAL_BROAD_CATCH` sites (the except blocks catching 7 exception types each). This is a pre-existing failure unrelated to this track.
---
@@ -147,8 +171,8 @@ The instrumentation was removed in commit `23862d35` (per `edit_workflow.md` §9
| `conductor/tracks/fix_mma_concurrent_tracks_sim_20260627/metadata.json` | New (track metadata) |
| `conductor/tracks/fix_mma_concurrent_tracks_sim_20260627/state.toml` | New (track state) |
| `src/app_controller.py` | Added `TrackMetadata` to import; changed `models.Metadata(...)` to `TrackMetadata(...)`; removed diagnostic instrumentation |
| `tests/mock_concurrent_mma.py` | Replaced session_id-based sprint routing with prompt-content-based routing |
| `docs/reports/OUTSTANDING_MMA_TEST_FAILURES_20260627.md` | (will be updated to RESOLVED in a follow-up commit) |
| `tests/mock_concurrent_mma.py` | Sprint routing now prompt-content-based (913aa48c); epic branch restructured to catch-all (fad1755b) |
| `docs/reports/OUTSTANDING_MMA_TEST_FAILURES_20260627.md` | Updated sections 4, 5, 6 to RESOLVED |
| `docs/reports/TRACK_COMPLETION_fix_mma_concurrent_tracks_sim_20260627.md` | New (this report) |
---
@@ -157,16 +181,16 @@ The instrumentation was removed in commit `23862d35` (per `edit_workflow.md` §9
1. **Run the full 11-tier batched test suite** to verify all tiers pass. The user should run this after merge review (per workflow.md "Prefer targeted tier runs"). Expected: 11/11 PASS (or 10/11 if the RAG flake is still the only remaining failure).
2. **Update `OUTSTANDING_MMA_TEST_FAILURES_20260627.md`** to mark the "UNRESOLVED" section as RESOLVED.
2. **Add `artifacts/` to `.gitignore`** as a follow-up. The mock counter file is at `artifacts/.mock_concurrent_mma_call_count` (project-tree, not under `tests/artifacts/`). This violates the `workspace_paths.md` rule that test workspaces should live under `tests/artifacts/`. The fix is to either move the file to `tests/artifacts/tier2_state/fix_mma_concurrent_tracks_sim_20260627/.mock_concurrent_mma_call_count` (and update the mock's path), or add `artifacts/` to `.gitignore` as a track-local test artifact directory.
3. **Add `artifacts/` to `.gitignore`** as a follow-up. The mock counter file is at `artifacts/.mock_concurrent_mma_call_count` (project-tree, not under `tests/artifacts/`). This violates the `workspace_paths.md` rule that test workspaces should live under `tests/artifacts/`. The fix is to either move the file to `tests/artifacts/tier2_state/fix_mma_concurrent_tracks_sim_20260627/.mock_concurrent_mma_call_count` (and update the mock's path), or add `artifacts/` to `.gitignore` as a track-local test artifact directory.
3. **Audit other `models.X` references in `src/`** for similar NameError regressions. A search for `models\.` in `src/` (excluding comments and `client.models` SDK attribute access) shows only the one site in `app_controller.py:4830`. So no other regressions of this type exist.
4. **Audit other `models.X` references in `src/`** for similar NameError regressions. A search for `models\.` in `src/` (excluding comments and `client.models` SDK attribute access) shows only the one site in `app_controller.py:4830`. So no other regressions of this type exist.
4. **Audit other literal-substring checks in `tests/`** for similar robustness issues. The stress test failure was caused by a literal-substring check in the mock. Are there other tests with similar patterns that might be fragile to test prompt variations?
---
## Conclusion
The MMA concurrent tracks test now passes consistently (5/5 runs). The parent branch `tier2/post_module_taxonomy_de_cruft_20260627` is now ready for merge after this fix track is reviewed.
Both MMA concurrent tracks tests (`test_mma_concurrent_tracks_execution` AND `test_mma_concurrent_tracks_stress`) now pass consistently (3/3 runs). The parent branch `tier2/post_module_taxonomy_de_cruft_20260627` is now ready for merge after this fix track is reviewed.
**Track SHIPPED.**
**Track SHIPPED (with the stress test fix).**