ai-game-2/docs/code-review-pr3.md

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.