## Code Review: PR 3 - "Stealth Cymbal"

### Scope
- **Branch:** stealth-cymbal
- **Base:** master
- **Files Changed:** 86 files (~1576 insertions, ~230 deletions)
- **Key Changes:** KQ4 Room Navigator skill, UID repair tools, MCP interaction server, room scene updates

### Review Team
- ✅ **ce-correctness-reviewer** - Logic errors, edge cases, state bugs
- ✅ **ce-testing-reviewer** - Coverage gaps, weak assertions, brittle tests
- ✅ **ce-maintainability-reviewer** - Coupling, complexity, naming, dead code
- ✅ **ce-project-standards-reviewer** - CLAUDE.md and AGENTS.md compliance
- ✅ **ce-agent-native-reviewer** - Agent-accessible design patterns
- ✅ **ce-learnings-researcher** - Past issues in docs/solutions/

---

### P1 -- High

| # | File | Issue | Reviewer | Confidence | Route |
|---|------|------|----------|------------|-------|
| 1 | `scenes/kq4_004_ogres_cottage/kq4_004_ogres_cottage.tscn:46` | UID mismatch - target UID for kq4_028_mine_entrance points to non-existent room | correctness | 75% | manual → downstream-resolver |
| 2 | `scripts/mcp_interaction_server.gd` | No runtime tests for 80+ TCP command handlers (171KB script) | testing | 50% | manual → downstream-resolver |

---

### P2 -- Moderate

| # | File | Issue | Reviewer | Confidence | Route |
|---|------|------|----------|------------|-------|
| 3 | `scenes/kq4_098_transitional_room/kq4_098_transitional_room.tscn` | Missing .uid companion file | correctness | 75% | manual → review-fixer |
| 4 | `scripts/build_room_graph.py:184` | `_resolve_target_room` returns None without explicit handling in callers | correctness | 75% | safe_auto → review-fixer |
| 5 | `tools/repair_uids.py:322` | Code duplication in fix_stale_target with inconsistent error handling | correctness | 75% | safe_auto → review-fixer |
| 6 | `tools/repair_uids.py` | No tests for file mutation operations (UID sync, target replacement) | testing | 75% | manual → downstream-resolver |
| 7 | `scripts/build_room_graph.py` | No tests for BFS edge cases (disconnected components, malformed UIDs) | testing | 75% | manual → downstream-resolver |
| 8 | `MainGame.gd`, `SetPiece_.gd` | No tests for signal emission logic and cursor action routing | testing | 50% | manual → downstream-resolver |
| 9 | `SetPiece_.gd:mock_interact()` | Unnecessary indirection - duplicates `_input()` logic | maintainability | 75% | gated_auto → downstream-resolver |
| 10 | `SetPiece_.gd` | Unused `entered/exited` signals with unclear `lab` param | maintainability | 75% | gated_auto → downstream-resolver |
| 11 | `tools/kq4_room_navigator.py` | sys.path manipulation creates coupling | maintainability | 50% | gated_auto → downstream-resolver |

---

### P3 -- Low

| # | File | Issue | Reviewer | Confidence | Route |
|---|------|------|----------|------------|-------|
| 12 | `MainGame.gd:13` | `get_current_room_name()` returns empty string on failure - silent failure mode | correctness | 50% | advisory → downstream-resolver |
| 13 | `scripts/mcp_interaction_server.gd:42` | Fixed 30-second timeout could mask real hangs | correctness | 50% | advisory → downstream-resolver |
| 14 | `SetPiece_.gd:60` | `mock_interact()` doesn't validate target room exists | correctness | 50% | advisory → downstream-resolver |
| 15 | `tools/kq4_room_navigator.py` | No tests for CLI tool | testing | 75% | advisory → human |
| 16 | `MainGame.gd` | Placeholder comment in `_ready()` - dead code | maintainability | 100% | advisory → human |
| 17 | `MainGame.gd` | Commented-out code with typo - dead code | maintainability | 100% | advisory → human |
| 18 | `scripts/mcp_interaction_server.gd` | Potentially incomplete implementation | maintainability | 50% | advisory → human |
| 19 | `.opencode/skills/kq4-room-navigator/SKILL.md:38-41` | Code block formatting changed from JSON to plain text | project-standards | 75% | advisory → human |
| 20 | `tools/kq4_room_navigator.py` | Dataclass overhead for Mismatch in CLI tool | maintainability | 50% | advisory → human |
| 21 | `.opencode/skills/kq4-room-navigator/SKILL.md` | Missing explicit retry guidance for `_busy` protocol | agent-native | 75% | advisory → human |
| 22 | `.opencode/skills/kq4-room-navigator/SKILL.md` | `mock_interact` vs `mock_interaction` typo in error messages | agent-native | 75% | safe_auto → review-fixer |
| 23 | `.opencode/skills/kq4-room-navigator/SKILL.md` | `--navigate` flag documented but not implemented in CLI | agent-native | 50% | gated_auto → downstream-resolver |

---

### Requirements Completeness
**No plan document found** for this PR. The PR title "Stealth Cymbal" is ambiguous and doesn't reference a specific plan.

---

### Pre-existing Issues
None identified in this review.

---

### Learnings & Past Solutions
**No institutional learnings found.** The `docs/solutions/` directory does not exist in this repository. Consider using `/ce-compound` skill after significant work to document:
- UID generation patterns (make_uid.py + .uid files)
- Navigation/pathfinding setup (NavigationServer2D)
- MCP server integration learnings
- Room transition wiring patterns

---

### Agent-Native Gaps
1. **Offline-Only Graph Discovery** - Agents can't dynamically discover room connectivity at runtime
2. **Manual Verification** - Navigation completion requires polling instead of signals
3. **No Path Validation** - Agents can attempt invalid navigation without guardrails
4. **Incomplete CLI** - The `--navigate` flag is documented but not implemented

---

### Residual Risks
1. **UID mismatch risk** - Room graph connectivity depends on UID consistency across all .tscn and .uid files. If UIDs drift from manual edits, pathfinding will fail even though runtime transitions work.
   - **Mitigation:** Run `tools/repair_uids.py --fix` before each significant room update. Consider integrating UID validation into CI pipeline.

2. **Timeout risk** - The MCP server's `_busy` flag uses a fixed 30-second timeout. If navigation animations or other operations take longer, commands may be prematurely reset.
   - **Mitigation:** Monitor `_busy` timeout warnings in logs. Consider making configurable or implementing per-command timeout overrides.

3. **Arbitrary code execution** - MCP eval command allows arbitrary code execution without guardrails.

4. **Heuristic risk** - UID auto-resolution in repair_uids.py may produce incorrect fixes based on heuristics.

---

### Testing Gaps
1. **No automated tests for UID consistency** across room transitions
2. **No tests for pathfinding edge cases** (disconnected rooms, single-room graphs, self-loops)
3. **No tests for MCP server command timeout** behavior under load
4. **No validation** that all TransitionPiece targets point to existing rooms
5. **No unit tests** for UID mismatch detection in repair_uids.py
6. **No tests** for BFS pathfinding edge cases in build_room_graph.py
7. **No tests** for SetPiece signal emission logic
8. **No tests** for busy-state timeout recovery in MCP server

---

### Coverage
- **Findings:** 23 total (2 P1, 9 P2, 12 P3)
- **Safe-auto (fixable now):** 3 findings
- **Manual (needs handoff):** 4 findings
- **Gated-auto (needs review):** 4 findings
- **Advisory (report-only):** 12 findings
- **Failed reviewers:** 0 of 6

---

### Verdict
**Ready with fixes**

**Fix order:**
1. **Critical:** Fix UID mismatch in kq4_004_ogres_cottage.tscn (finding #1) - this breaks navigation
2. **High:** Add missing .uid file for kq4_098_transitional_room (finding #3)
3. **Medium:** Fix None handling in build_room_graph.py (finding #4)
4. **Medium:** Consolidate code duplication in repair_uids.py (finding #5)
5. **Low:** Address dead code and formatting issues (findings #12-23)

**Before merge:** Run `tools/repair_uids.py --fix` to synchronize all UIDs and verify the UID mismatch is resolved.