feat(m2): wire hook listener + save exchange into BridgeLoop, add city_id#11
Conversation
…y_id The bridge daemon is now end-to-end: hook events drive state mutations, each turn the scoring engine produces rewards, and the save exchange persists the result for the Unciv mod / TUI to poll. bridge/bridge_loop.py — BridgeLoop now takes optional hook_listener and save_exchange. process_turn() ticks idle checks, scores, persists, and fires per-city callbacks. start_with_hooks() runs both the turn loop and the listener's stdin loop concurrently — production entrypoint. bridge/session_state.py — Session gains `city_id` field (defaulted to "" for the M0/M1 single-city slice; preserved through SaveExchange round-trip). bridge/scoring.py — adds calculate_per_city_rewards() returning Dict[city_id, rewards]. Sessions without a city_id bucket under "". bridge/tests/test_city_and_loop_integration.py — 12 tests covering: - city_id default / round-trip / serialization - per-city scoring grouping, unmapped-bucketing, sum-equals-global - BridgeLoop persists via SaveExchange on each turn with active sessions - BridgeLoop skips persist when no active sessions (no spurious writes) - feed_event proxies to listener; raises if no listener - per_city_rewards callback fires - end-to-end: hook stream → goals/plans → real scoring numbers reflect the session activity AND survive a save/reload round trip Total: 77 passing. Toward kanban t_859cbdbf (M2 Core Economy) and t_cefabee5 (M1 Vertical Slice — city-id mapping was an outstanding M1 chunk). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the BridgeLoop to orchestrate hook ingestion, scoring, and persistence, while introducing a city_id field to the Session model to support per-city reward calculations. Feedback focuses on correcting a logic error where global rewards were incorrectly recorded for individual sessions, optimizing performance by consolidating loops over active sessions, and improving code maintainability by refactoring hardcoded reward structures and using more idiomatic object instantiation.
| per_city = self.scoring.calculate_per_city_rewards(active_sessions) | ||
|
|
||
| for session in active_sessions: | ||
| self.scoring.record_history(session, rewards) |
There was a problem hiding this comment.
The rewards variable contains the aggregate rewards for all active sessions in the turn. Recording this global total into each individual session's history is a logic error that will result in incorrect historical data. Each session should record its own specific reward contribution.
| self.scoring.record_history(session, rewards) | |
| session_reward = self.scoring.calculate_session_reward(session) | |
| self.scoring.record_history(session, session_reward) |
| rewards = self.scoring.calculate_turn_rewards(active_sessions) | ||
| per_city = self.scoring.calculate_per_city_rewards(active_sessions) | ||
|
|
||
| for session in active_sessions: | ||
| self.scoring.record_history(session, rewards) |
There was a problem hiding this comment.
The current implementation performs multiple passes over the active_sessions list (once in calculate_turn_rewards, once in calculate_per_city_rewards, and once for history recording). For better performance and to avoid redundant calculations, consider consolidating these into a single pass that calculates all required metrics simultaneously.
| rewards = self.calculate_session_reward(session) | ||
| city = session.city_id or "" | ||
| bucket = per_city.setdefault( | ||
| city, {"gold": 0, "science": 0, "culture": 0, "production": 0} |
There was a problem hiding this comment.
The reward dictionary structure {"gold": 0, "science": 0, "culture": 0, "production": 0} is hardcoded here and in calculate_turn_rewards. To improve maintainability and prevent inconsistencies if the reward types change, consider defining a constant or a helper method to provide this initial state.
| last_reward=session_data.get("last_reward") | ||
| ) | ||
| session.city_id = session_data.get("city_id", "") |
There was a problem hiding this comment.
The city_id field can be passed directly to the Session constructor instead of being assigned after the object is created. This is more idiomatic and consistent with how other fields are handled in from_dict.
| last_reward=session_data.get("last_reward") | |
| ) | |
| session.city_id = session_data.get("city_id", "") | |
| last_reward=session_data.get("last_reward"), | |
| city_id=session_data.get("city_id", "") | |
| ) |
…eholder Closes audit P1 acceptance criteria #11 (README GIF placeholder) and #12 (documentation covers all four contribution paths). CONTRIBUTING.md — first-class onboarding doc for new contributors: - Enumerates the four paths: Bridge (Python), TUI (Python/Textual), Unciv mod (JSON+Lua), Documentation (Markdown). Each with a clear files/entrypoint listing and "good for you if" hook. - Reproduces the test ownership matrix from PRD §"Test Ownership Matrix" and points at each path's test file. - PR process aligned with the project's safety rules (no force push to main, no --no-verify, branch naming convention). - Lists standing `good first issue`-style options per path so acceptance #13 has a path forward even before labeled issues exist. - Code style for each language (Python, Markdown, Lua, JSON). README.md — adds a GIF placeholder block at the top with the recording protocol pointer (`docs/media/README.md`). Acceptance criterion #11 calls out a *working* GIF; this lands the placeholder + recording spec so it can be filled in at M4 launch time when the Unciv map is authored. The placeholder is explicit, not deceptive — readers know it's a planned asset. docs/media/README.md — recording protocol for the V1 GIF: 30-second shot list, frame timing, ffmpeg + gifski pipeline. Documents WHY the GIF can't ship today (needs M4 map content first). No code changes. 191 tests still passing. Refs: CTO audit doc PR #22, kanban t_26404be3, acceptance #11 + #12. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
The bridge daemon is now end-to-end: hook events drive state mutations, each turn the scoring engine produces rewards, and the save exchange persists state for downstream consumers.
BridgeLoopaccepts optionalhook_listenerandsave_exchange;process_turn()ticks idle checks, scores, persists, firesturn_completeandper_city_rewardscallbacks.start_with_hooks()runs the turn loop and the listener's stdin loop concurrently (production entrypoint).Sessiongainscity_id(PRD: cities ↔ sessions); preserved through SaveExchange round-trip.ScoringEngine.calculate_per_city_rewards()groups by city_id; unmapped sessions bucket under "".Toward
Kanban:
t_859cbdbf(M2 Core Economy) +t_cefabee5(M1 Vertical Slice — city-id mapping was an outstanding M1 chunk)Test plan
pytest -q→ 77 passed (was 65; adds 12)Remaining for M2 (out of this PR)
🤖 Generated with Claude Code