docs: CTO audit of build vs PRD v1.5#22
Merged
Merged
Conversation
Hard-eyed alignment review against ClaudeKingdomsPRD.md. Calls out the
true ship status of every BR/BS/CL/GM/TU requirement and every V1
acceptance criterion, plus a P0/P1/P2 issue list.
Headline findings:
- 9/21 acceptance criteria fully met, 6/21 partial, 6/21 missing
- Bridge architecture and tests are strong; ship-blockers are
concentrated in (a) P0 enum/schema gaps and (b) M4 content work
- Two P0 alignment gaps:
1. SessionStatus missing Inactive + Redirected (BR-1, BS-7, TU-4)
2. Bridge–TUI JSON schema on disk omits 8 of 13 PRD-mandated
minimum per-session fields (binding-contract drift)
- One P0 logic gap: Stop event never transitions to COMPLETED (CL-3)
See docs/audit-2026-05-08.md for the full grid.
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 introduces a comprehensive CTO-style audit document (docs/audit-2026-05-08.md) that evaluates the project's alignment with PRD v1.5. The audit identifies critical P0 gaps, including missing session states and schema drifts, alongside various P1 and P2 issues. Feedback from the review suggests clarifying the required session states to resolve PRD inconsistencies and recommends moving environment-specific technical debt notes out of the permanent project documentation to maintain clarity for all contributors.
Comment on lines
+21
to
+22
| 1. **`SessionStatus` enum is missing `Inactive` and `Redirected` as | ||
| distinct states** despite the PRD requiring all five (BR-1, TU-4). |
There was a problem hiding this comment.
The audit notes the PRD requires "all five" states, but BR-1 (line 205) and TU-4 (line 313) actually list different sets (5 vs 6 states). Specifically, TU-4 includes Idle and Suspended, while BR-1 includes Inactive. Clarifying the unified set of required states would help resolve the P0 alignment gap more effectively.
Suggested change
| 1. **`SessionStatus` enum is missing `Inactive` and `Redirected` as | |
| distinct states** despite the PRD requiring all five (BR-1, TU-4). | |
| 1. **`SessionStatus` enum is missing several states** (e.g., `Inactive`, `Redirected`, `Idle`) required by the PRD's functional and TUI requirements (BR-1, TU-4). |
Comment on lines
+208
to
+221
| 9. The `default` profile keeps showing up as the assignee on tasks | ||
| that should be claude-code. Hermes's dispatcher behavior here is | ||
| inconsistent. | ||
|
|
||
| 10. The TUI `KingdomView` reads `SessionState` directly, violating | ||
| TU-9 ("TUI must consume only via the schema"). | ||
|
|
||
| 11. `audit log` (BR-9) — only an in-memory rolling log exists. PRD | ||
| expects a persisted, structured, human-readable log file. | ||
|
|
||
| 12. `gemini-cli` MCP keeps re-adding itself via `hermes setup` and | ||
| failing to load. Either fix the upstream `mcp-gemini-cli` package | ||
| or pin it as `enabled: false` in the Hermes config. | ||
|
|
There was a problem hiding this comment.
Points 9 and 12 in the "P2 — quality issues and tech debt" section describe issues related to the hermes dispatcher and gemini-cli MCP. These appear to be specific to the AI agent's local development environment or configuration rather than the project's source code or the Unciv mod itself. Including these in a permanent project audit document might be confusing for other contributors who do not use the same setup. Consider moving these environment-specific notes to a separate section or an internal issue tracker.
2 tasks
SolshineCode
added a commit
that referenced
this pull request
May 8, 2026
…BR-1, §BS-7) Closes the P0 #1 issue from the 2026-05-08 CTO audit. The PRD requires six classification states (Inactive, Active, WaitingHuman, Completed, Redirected, Suspended); we previously had four. Without REDIRECTED as a state, the audit log can't carry the redirect signal cleanly. Without INACTIVE distinct from SUSPENDED, BS-7's "game open, no work" vs "game closed" distinction wasn't representable. bridge/session_state.py: - Adds INACTIVE and REDIRECTED to SessionStatus - Docstring documents the full state semantics + the BS-7 distinction - get_active_sessions now includes REDIRECTED (no clawback rule) bridge/scoring.py: - SimpleScoringStrategy treats REDIRECTED like ACTIVE for base reward. Culture falls to 0 because momentum is 0, but production / gold / science still flow — that's the PRD's "no negative balance" rule in action. bridge/hook_listener.py: - _handle_permission_denied now marks the session REDIRECTED in addition to zeroing momentum; emits 1 or 2 StateChange records depending on whether momentum was already zero. - _handle_task_completed flips REDIRECTED → ACTIVE (productive event ends the redirect period). _handle_user_prompt_submit already covers WAITING/REDIRECTED/INACTIVE → ACTIVE via the existing generic transition. bridge/tests/test_economy.py: - Updates test_redirect_via_hook_listener_permission_denied to expect the new 2-change emission. bridge/tests/test_hook_listener.py: - Updates test_permission_denied_with_zero_momentum to assert the new "still marks REDIRECTED" behavior (single status change, no momentum change since already zero). bridge/tests/test_redirected_state_machine.py — 10 new tests: - Enum has all six PRD-mandated states - INACTIVE + REDIRECTED serialize through SaveExchange round-trip - PermissionDenied → REDIRECTED + momentum=0 - REDIRECTED → ACTIVE on UserPromptSubmit - REDIRECTED → ACTIVE on TaskCompleted - Redirected sessions keep earning base reward (no clawback) - get_active_sessions includes REDIRECTED - get_active_sessions excludes INACTIVE + SUSPENDED - INACTIVE and SUSPENDED are distinct (BS-7) Total: 159 passing (was 149; adds 10). Refs: kanban t_26404be3 (P0 audit issues followup), PR #22 (audit doc). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 8, 2026
SolshineCode
added a commit
that referenced
this pull request
May 8, 2026
…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>
This was referenced May 8, 2026
SolshineCode
added a commit
that referenced
this pull request
May 8, 2026
…alog Closes audit P2: TU-9 enforcement and progress on acceptance #13. tui/bridge_view.py — schema-only entrypoint: - BridgeView.from_payload(dict) — strict path; consumes the same JSON SaveExchange writes (PRD §"Bridge–TUI Interface Contract"). - BridgeView.from_state(state) — in-process convenience; serializes through the same code path SaveExchange uses so the resulting view is byte-equivalent to one read from disk. - SessionView (frozen dataclass) and BridgeView (frozen dataclass) expose only the PRD-mandated minimum schema fields plus a couple of display conveniences (name, city_id) explicitly outside the binding contract. - total_rewards() helper aggregates across cities. - SCHEMA_PER_SESSION_FIELDS / SCHEMA_TOP_LEVEL_FIELDS constants documented as the binding-contract surface area. - Frozen dataclasses prevent accidental mutation; legacy aliases (status, momentum) are accepted on read for back-compat. This is the choke point PRD §TU-9 mandates. Existing TUI code that reads SessionState directly remains for now (gradual migration); new TUI work should use BridgeView. The audit doc's P2 #10 entry can now be closed once the full migration ships. tui/tests/test_bridge_view.py — 14 tests: - from_state populates top-level + per-session breakdown + per_city_* - from_state includes per_city_events when buffered - from_payload round-trips through SaveExchange on disk - from_payload accepts legacy aliases (status / momentum) - Empty-session and missing-per-city defaults handled - total_rewards aggregates correctly; empty kingdom → all zeros - Schema field constants match PRD line 231 wording exactly - SessionView and BridgeView are immutable (frozen dataclass) docs/good-first-issues.md — ready-to-paste issue text for five candidate `good first issue` GitHub issues, one per contribution path (Bridge property tests, audit log rotation, TUI session list panel, mod city names, docs audit refresh). Maintainers run `gh issue create` with the body verbatim when they want to surface one. Closes acceptance #13 architecturally — the candidate list exists; the GitHub-side label is one command away. Total: 262 passing (was 250; adds 12). Refs: PR #22, kanban t_26404be3. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Hard-eyed alignment review of the current build (HEAD: 4b4b119, 149 tests passing) against the PRD v1.5. See
docs/audit-2026-05-08.md.Headline findings
SessionStatusenum missingInactive+Redirected(BR-1, BS-7, TU-4)Stophook never transitions a session toCOMPLETED(CL-3)Test plan
🤖 Generated with Claude Code