docs/code-review-pr3.md

@@ -0,0 +1,135 @@
+## 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.