Add planning artifacts for setup verification #286
Conversation
Summary of ChangesHello @3abr, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes foundational project management and technical documentation for the MuMain client. It lays out the project's scope, future development phases, and current status, while also providing deep insights into the existing codebase's design, potential issues, and development practices. The addition of an automated build script further enhances the project's readiness for development by simplifying the initial setup and compilation process. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 adds a comprehensive set of planning artifacts and a build script for setup verification. The documentation is thorough and well-structured. I've identified a couple of areas for improvement. In the PowerShell build script, hardcoded output paths could be made more robust by dynamically parsing CMakePresets.json. Additionally, one of the planning documents contains hardcoded absolute local file paths, which should be updated to be portable for other developers.
There was a problem hiding this comment.
Pull request overview
Adds project planning artifacts for Phase 1 setup verification and a Windows helper script to build the required CMake preset variants and configure server connection settings.
Changes:
- Added
build-and-configure.ps1to configure/build all four Windows Release presets and writeconfig.iniinto build outputs. - Added Phase 1 planning artifacts (
CONTEXT,PLAN) and updated planning state/roadmap/project context. - Added
.planning/codebase/documentation describing stack, architecture, structure, conventions, testing, integrations, and concerns.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| build-and-configure.ps1 | Automates Windows preset builds and writes connection settings to config.ini in build outputs. |
| .planning/phases/01-setup-verification/01-CONTEXT.md | Captures assumptions/scope for Phase 1 setup verification. |
| .planning/phases/01-setup-verification/01-01-PLAN.md | Execution plan/checklist for building 4 variants and doing a manual run/connect verification. |
| .planning/config.json | Configures planning workflow behavior (interactive mode, gates, parallelization). |
| .planning/codebase/TESTING.md | Documents current absence of automated tests and CI build-only validation. |
| .planning/codebase/STRUCTURE.md | Summarizes repository layout and key directories/files. |
| .planning/codebase/STACK.md | Documents languages, build tooling, and key dependencies. |
| .planning/codebase/INTEGRATIONS.md | Documents OpenMU server integration and the .NET network bridge. |
| .planning/codebase/CONVENTIONS.md | Captures observed coding/style conventions across C++/C#. |
| .planning/codebase/CONCERNS.md | Lists known tech debt, fragile areas, and security/perf concerns. |
| .planning/codebase/ARCHITECTURE.md | High-level architectural overview and layer responsibilities. |
| .planning/STATE.md | Tracks current planning phase/progress state. |
| .planning/ROADMAP.md | Defines phase roadmap and success criteria. |
| .planning/PROJECT.md | Project goals, constraints, and key decisions summary. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @C:\Users\Uburrato\.claude/get-shit-done/workflows/execute-plan.md | ||
| @C:\Users\Uburrato\.claude/get-shit-done/templates/summary.md | ||
| @C:\Users\Uburrato\.claude/get-shit-done/references/checkpoints.md |
There was a problem hiding this comment.
The execution_context references absolute local paths under C:\Users\Uburrato\..., which won’t exist for other contributors and makes the plan non-portable. Use repo-relative paths (checked into the repo) or describe the workflow/template names without machine-specific locations.
| @C:\Users\Uburrato\.claude/get-shit-done/workflows/execute-plan.md | |
| @C:\Users\Uburrato\.claude/get-shit-done/templates/summary.md | |
| @C:\Users\Uburrato\.claude/get-shit-done/references/checkpoints.md | |
| @.planning/workflows/execute-plan.md | |
| @.planning/templates/summary.md | |
| @.planning/references/checkpoints.md |
| **Requirements**: Build pipeline | ||
| **Success Criteria**: | ||
| 1. `cmake --preset windows-x86` configures successfully | ||
| 2. `cmake --build --preset windows-x86-debug` produces Main.exe |
There was a problem hiding this comment.
Phase 1 success criteria references the windows-x86-debug build preset, but the Phase 1 context/plan focuses on building Release variants (and MuEditor Release). Align the roadmap success criteria with the plan (either update to Release presets or explicitly call out both Debug and Release).
| 2. `cmake --build --preset windows-x86-debug` produces Main.exe | |
| 2. `cmake --build --preset windows-x86-debug` and the corresponding Release preset(s) (including MuEditor Release) produce the expected executable(s) |
| $ConfigContent = @" | ||
| [CONNECTION SETTINGS] | ||
| ServerIP=$ServerIP | ||
| ServerPort=$ServerPort | ||
|
|
||
| "@ | ||
|
|
||
| Write-Host "" | ||
| Write-Host "=== Configuring config.ini ===" -ForegroundColor Cyan | ||
| foreach ($dir in $OutputDirs) { | ||
| $configPath = Join-Path $RepoRoot (Join-Path $dir "config.ini") | ||
| $dirPath = Split-Path $configPath -Parent | ||
| if (Test-Path $dirPath) { | ||
| Set-Content -Path $configPath -Value $ConfigContent -Encoding UTF8 |
There was a problem hiding this comment.
The script builds outputs from src/bin/ which already includes a full config.ini (window/audio/login defaults). Writing a new file with only [CONNECTION SETTINGS] will clobber any existing per-user settings (and can remove other sections if they become relevant later). Consider preserving the existing config.ini and only updating ServerIP/ServerPort, or start from the repo’s src/bin/config.ini template and edit those two keys.
| $ConfigContent = @" | |
| [CONNECTION SETTINGS] | |
| ServerIP=$ServerIP | |
| ServerPort=$ServerPort | |
| "@ | |
| Write-Host "" | |
| Write-Host "=== Configuring config.ini ===" -ForegroundColor Cyan | |
| foreach ($dir in $OutputDirs) { | |
| $configPath = Join-Path $RepoRoot (Join-Path $dir "config.ini") | |
| $dirPath = Split-Path $configPath -Parent | |
| if (Test-Path $dirPath) { | |
| Set-Content -Path $configPath -Value $ConfigContent -Encoding UTF8 | |
| function Update-ConfigIni { | |
| param( | |
| [string]$Path, | |
| [string]$ServerIP, | |
| [int]$ServerPort | |
| ) | |
| # If config.ini does not exist yet, create a minimal one with connection settings. | |
| if (-not (Test-Path $Path)) { | |
| $content = @" | |
| [CONNECTION SETTINGS] | |
| ServerIP=$ServerIP | |
| ServerPort=$ServerPort | |
| "@ | |
| Set-Content -Path $Path -Value $content -Encoding UTF8 | |
| return | |
| } | |
| # Load existing config.ini and update only the connection settings. | |
| $lines = Get-Content -Path $Path | |
| $inConnectionSection = $false | |
| $foundSection = $false | |
| for ($i = 0; $i -lt $lines.Count; $i++) { | |
| $line = $lines[$i] | |
| if ($line -match '^\s*\[.*\]') { | |
| # Enter/leave [CONNECTION SETTINGS] section | |
| $inConnectionSection = $line -match '^\s*\[CONNECTION SETTINGS\]\s*$' | |
| if ($inConnectionSection) { | |
| $foundSection = $true | |
| } | |
| continue | |
| } | |
| if ($inConnectionSection) { | |
| if ($line -match '^\s*ServerIP\s*=') { | |
| $lines[$i] = "ServerIP=$ServerIP" | |
| } elseif ($line -match '^\s*ServerPort\s*=') { | |
| $lines[$i] = "ServerPort=$ServerPort" | |
| } | |
| } | |
| } | |
| if (-not $foundSection) { | |
| # Append a new [CONNECTION SETTINGS] section if it was not present. | |
| $lines += "" | |
| $lines += "[CONNECTION SETTINGS]" | |
| $lines += "ServerIP=$ServerIP" | |
| $lines += "ServerPort=$ServerPort" | |
| $lines += "" | |
| } else { | |
| # Ensure the keys exist in the section even if they were not present before. | |
| $inConnectionSection = $false | |
| $hasServerIP = $false | |
| $hasServerPort = $false | |
| for ($i = 0; $i -lt $lines.Count; $i++) { | |
| $line = $lines[$i] | |
| if ($line -match '^\s*\[.*\]') { | |
| if ($inConnectionSection) { | |
| break | |
| } | |
| $inConnectionSection = $line -match '^\s*\[CONNECTION SETTINGS\]\s*$' | |
| continue | |
| } | |
| if ($inConnectionSection) { | |
| if ($line -match '^\s*ServerIP\s*=') { | |
| $hasServerIP = $true | |
| } elseif ($line -match '^\s*ServerPort\s*=') { | |
| $hasServerPort = $true | |
| } | |
| } | |
| } | |
| if (-not $hasServerIP -or -not $hasServerPort) { | |
| # Append missing keys at the end of the file to avoid disturbing existing structure. | |
| if (-not $hasServerIP) { | |
| $lines += "ServerIP=$ServerIP" | |
| } | |
| if (-not $hasServerPort) { | |
| $lines += "ServerPort=$ServerPort" | |
| } | |
| $lines += "" | |
| } | |
| } | |
| Set-Content -Path $Path -Value $lines -Encoding UTF8 | |
| } | |
| Write-Host "" | |
| Write-Host "=== Configuring config.ini ===" -ForegroundColor Cyan | |
| foreach ($dir in $OutputDirs) { | |
| $configPath = Join-Path $RepoRoot (Join-Path $dir "config.ini") | |
| $dirPath = Split-Path $configPath -Parent | |
| if (Test-Path $dirPath) { | |
| Update-ConfigIni -Path $configPath -ServerIP $ServerIP -ServerPort $ServerPort |
| $configPath = Join-Path $RepoRoot (Join-Path $dir "config.ini") | ||
| $dirPath = Split-Path $configPath -Parent | ||
| if (Test-Path $dirPath) { | ||
| Set-Content -Path $configPath -Value $ConfigContent -Encoding UTF8 |
There was a problem hiding this comment.
Set-Content -Encoding UTF8 can emit a UTF-8 BOM in Windows PowerShell 5.1. Since the client reads config.ini via Windows INI APIs (GetPrivateProfileStringW), a BOM at the start of the file can prevent the first section header from matching, causing the ServerIP/ServerPort values to be ignored. Use an encoding compatible with the INI API (e.g., ASCII) or explicitly write UTF-8 without BOM / UTF-16LE with BOM.
| Set-Content -Path $configPath -Value $ConfigContent -Encoding UTF8 | |
| Set-Content -Path $configPath -Value $ConfigContent -Encoding ASCII |
| param( | ||
| [string]$ServerIP = "192.168.3.34", | ||
| [int]$ServerPort = 44406 |
There was a problem hiding this comment.
Defaulting -ServerIP to 192.168.3.34 is environment-specific and doesn’t match the project’s default config values (e.g., src/bin/config.ini and CfgDefaultServerIP). Consider defaulting to the project default (or localhost) and requiring users to pass -ServerIP when targeting a LAN server.
1 participant
No description provided.