diff --git a/.github/skills/implementing-plan-steps/SKILL.md b/.github/skills/implementing-plan-steps/SKILL.md index b17dabe..509316a 100644 --- a/.github/skills/implementing-plan-steps/SKILL.md +++ b/.github/skills/implementing-plan-steps/SKILL.md @@ -1,6 +1,6 @@ --- name: implementing-plan-steps -description: Implements the next unchecked step of a Markdown implementation plan in an isolated git worktree, reads relevant repository memory, adds tests, verifies the build, reviews memory for useful updates, updates the plan checklist, commits, and opens a pull request against the development branch. Use when the user asks to implement, continue, or work on the next step of a plan file. +description: Implements the next unchecked step of a Markdown implementation plan in an isolated git worktree, reads relevant repository memory, adds tests, verifies the build, reviews memory for useful updates, updates the plan checklist, commits, and opens a pull request against the development branch or an open previous-phase PR for stacked work. Use when the user asks to implement, continue, or work on the next step of a plan file. argument-hint: Path to the plan file (e.g. _plans/My-Feature.md) disable-model-invocation: true --- @@ -46,16 +46,25 @@ Copy this checklist and track progress: **2. Select the step.** Read the plan. Pick the first unchecked item in its checklist. If all items are checked, report that and stop. -Record the supplied plan's path and contents before creating the worktree. For a repository-local plan, record its repository-relative path. If its supplied version is untracked, uncommitted, or differs from the version on `development`, copy that version to the same path in the worktree before switching directories. The worktree copy is the plan used and updated in step 8. For a plan outside the repository, keep using and updating its supplied original path, and leave it uncommitted. +Record the supplied plan's path and contents before creating the worktree. For a repository-local plan, record its repository-relative path. If its supplied version is untracked, uncommitted, or differs from the version on the selected base branch, copy that version to the same path in the worktree before switching directories. The worktree copy is the plan used and updated in step 8. For a plan outside the repository, keep using and updating its supplied original path, and leave it uncommitted. -**3. Create worktree.** +**3. Choose the base and create the worktree.** + +Check whether the previous phase's pull request is still open. If it is, use +that PR's head branch as the base so this step becomes a stacked PR. Otherwise, +use `development`. If the matching previous-phase PR cannot be identified +unambiguously, ask the user which PR to stack on before creating the worktree. +Fetch the selected base branch from `origin` and use it for the worktree: ```bash -git fetch origin -git worktree add -b .worktrees/ origin/development +git fetch origin +git worktree add -b .worktrees/ origin/ ``` -``: short kebab-case name derived from the step. Work only inside the worktree from here on. +`` is `development` when there is no open previous-phase PR, or +the head branch of that open PR. ``: short kebab-case name derived +from the step. Record the selected base branch for the PR in step 10. Work only +inside the worktree from here on. Before switching directories, ensure the supplied repository-local plan is present at its recorded repository-relative path in the worktree. Copy the supplied version there if the worktree does not contain that version. @@ -76,7 +85,7 @@ Compare warnings with the latest known `Debug | x64` baseline for the same toolc **9. Commit.** Before committing, check that `README.md` and the Markdown files in `docs/` are up to date for the selected step; update directly related documentation as needed. Make one commit containing code, tests, relevant documentation and memory changes, and the plan update if the plan is repository-local. Follow [`.github/commit-conventions.instructions.md`](../../commit-conventions.instructions.md), including its gitmoji, conventional type and scope, and description-list format. Example subject: `✨ feat(core): add memory store serialization`. -**10. Push and open PR** against `development`: +**10. Push and open PR** against the base branch selected in step 3: - Title: the plan title. - Body: motivation, context, key design decisions, dependencies. Short. No filler or empty sections. - Mention memory changes briefly if any were made. @@ -85,7 +94,7 @@ Compare warnings with the latest known `Debug | x64` baseline for the same toolc ```bash git push -u origin -gh pr create --base development --title "" --body-file +gh pr create --base --title "" --body-file ``` If a build or test still fails after three focused fix-and-rerun attempts, stop and report the failure rather than looping indefinitely. diff --git a/.github/skills/reviewing-pull-requests/SKILL.md b/.github/skills/reviewing-pull-requests/SKILL.md new file mode 100644 index 0000000..ee4dabd --- /dev/null +++ b/.github/skills/reviewing-pull-requests/SKILL.md @@ -0,0 +1,188 @@ +--- +name: reviewing-pull-requests +description: "Reviews a GitHub pull request as a senior staff engineer and posts findings as line comments without approving. Use when reviewing a MapWinGIS6 PR, doing a code review, or checking changes against a base branch, including C++ core, ActiveX/COM, GDAL, MSBuild/vcpkg and GoogleTest changes." +argument-hint: " [base branch]" +--- + +# PR review + +## Inputs + +- PR: a number resolves to `https://github.com/MapWindow/MapWinGIS6/pull/`; a full URL also works. If missing, ask. +- Base branch: the PR's `baseRefName` (usually `development`). If an optional base argument is provided, it must match `baseRefName`; stop if it does not. + +You are a Senior Staff Software Engineer performing a mandatory review. A human reviewer reads your findings, inspects the code too, and decides what to act on. + +## Workflow + +Copy this checklist and track progress: + +``` +- [ ] 1. Load PR metadata, linked issues and repo guidelines +- [ ] 2. Fetch diff, PR head and existing comments +- [ ] 3. Review every changed file +- [ ] 4. Post findings as one review +- [ ] 5. Verify the review posted +- [ ] 6. Reply with the summary +``` + +**1. Load PR metadata** + +```bash +gh pr view --repo MapWindow/MapWinGIS6 --json title,body,baseRefName,headRefOid,files,commits,closingIssuesReferences +``` + +If the PR links or references GitHub issues (`#123`, `Fixes #123`), read them for intent and acceptance criteria: + +```bash +gh issue view --repo MapWindow/MapWinGIS6 --comments +``` + +Read the repo guidelines at the base branch and review against them: `AGENTS.md`, `.github/copilot-instructions.md` (if present), and the relevant files in `docs/` (coding standards, architecture, building, testing, GDAL integration, COM interface versioning). + +```bash +git fetch origin +git show origin/:AGENTS.md +git ls-tree --name-only origin/ docs/ +``` + +**2. Fetch diff, PR head and existing comments** + +```bash +gh pr diff --repo MapWindow/MapWinGIS6 +git fetch origin +pull//head:pr- +git show pr-: # full file at PR head +gh api repos/MapWindow/MapWinGIS6/pulls//comments --paginate --jq '.[] | {path, line, original_line, body}' +gh api repos/MapWindow/MapWinGIS6/pulls//reviews --paginate --jq '.[] | {state, body}' +gh api repos/MapWindow/MapWinGIS6/issues//comments --paginate --jq '.[] | .body' +gh pr checks --repo MapWindow/MapWinGIS6 +``` + +Do not check out the branch; leave the working tree untouched. + +**3. Review** every changed file using the [review rules](#review-rules). Read surrounding code (headers and their implementations, callers, COM IDL/interfaces, ActiveX wrappers over the core, tests, `.vcxproj`/`.props`/`.targets`, `vcpkg.json`, workflows) when needed to judge correctness. Treat failing CI checks as findings when the PR causes them. + +Before posting, drop any finding already raised in an existing comment (by you, another bot or a human), matched on same file and same issue, even if the line moved or the wording differs. Do not repost on re-review. If an earlier comment is still unresolved and the new commits make it worse, reply to that thread instead of adding a new comment: + +```bash +gh api repos/MapWindow/MapWinGIS6/pulls//comments//replies --method POST -f body='...' +``` + +**4. Post** all findings as a single review with event `COMMENT`. Write the payload to a temp file outside the repo: + +```json +{ + "commit_id": "", + "event": "COMMENT", + "body": "Automated review. Final decision is with the human reviewer.", + "comments": [ + { + "path": "src/MapWinGIS.Core/src/Layer.cpp", + "line": 42, + "side": "RIGHT", + "body": "..." + }, + { + "path": "src/MapWinGIS.Core/include/mapwingis/Layer.h", + "start_line": 10, + "line": 14, + "side": "RIGHT", + "body": "..." + } + ] +} +``` + +```bash +gh api repos/MapWindow/MapWinGIS6/pulls//reviews --method POST --input +``` + +On `422` (line outside a diff hunk): move that finding to the nearest changed line in the same file, or into the review `body` as `file:line`. Retry until the review posts. + +**5. Verify** the posted line-comment count matches your line findings: + +```bash +gh api repos/MapWindow/MapWinGIS6/pulls//reviews//comments --paginate --slurp --jq 'map(length) | add' +``` + +**6. Reply** with the [final summary](#final-summary). + +## Review rules + +- Report every issue, including low-severity and uncertain ones. Do not filter; filtering happens afterwards using severity and confidence. Real defects outrank style, but list both. +- Be sceptical: hunt for hidden failure scenarios, edge cases, race conditions, partial-failure states and operational risks, not just what the diff obviously does. + +Focus areas: + +- Functional correctness, including geometry/coordinate edge cases (empty or null geometries, projections, extents, precision) +- Memory and resource safety: ownership, RAII, lifetimes, dangling references, leaks, double frees, buffer overruns, integer overflow, uninitialised values, undefined behaviour +- GDAL/OGR usage: handle ownership and release (`GDALClose`, `OGRFeature::DestroyFeature`, smart-pointer deleters), error checking of return codes and `CPLGetLastErrorMsg`, driver availability, progress/cancellation callbacks +- Layering: `MapWinGIS.Core` stays free of COM, ATL and MFC; the ActiveX layer remains a thin wrapper over the core +- COM/ActiveX correctness: `HRESULT` handling, reference counting, `BSTR`/`VARIANT`/`SAFEARRAY` lifetimes, threading apartments, event firing, IDL/interface versioning and registration +- Public API and ABI: headers in `include/mapwingis/`, export macros, exceptions crossing DLL/COM boundaries, deliberate vs accidental breaks with MapWinGIS 5 behaviour +- Concurrency: data races, locking, async operations and cancellation, thread-safety of shared state such as the viewport +- Error handling and exception safety +- Performance: unnecessary copies, allocations in hot paths (rendering, feature iteration), algorithmic complexity +- Build and platform: Win32 and x64, Debug and Release, `.vcxproj`/`Directory.Build.*` changes, warnings, vcpkg manifest and triplet changes +- Security: untrusted file/path input, format-string issues, unchecked sizes from external data +- Test coverage: missing or weak GoogleTest cases in `tests/MapWinGIS.Core.Tests` or `tests/MapWinGIS.ActiveX.Tests` for new or changed behaviour +- Conformance with `AGENTS.md` and `docs/` coding standards (modern C++, naming, includes, `const`-correctness) +- Documentation: `docs/`, `README.md` and samples updated when behaviour or API changes +- Maintainability and readability +- Code comments: are new or updated comments useful long-term, or temporary/obsolete/restating the code? + +## Finding format + +Each finding is a line comment on the exact relevant line(s), in English: + +- **Severity:** Critical | Major | Minor | Suggestion +- **Confidence:** High | Medium | Low +- **Issue:** what is wrong. +- **Impact:** what breaks or what risk it creates, and under which conditions. +- **Fix:** a concrete improvement; add a GitHub `suggestion` block when practical. + +Example: + +````markdown +**Severity:** Major | **Confidence:** High + +**Issue:** The dataset returned by `GDALOpenEx` is stored in a raw pointer and never closed on the early-return path. +**Impact:** Each failed layer load leaks the dataset and keeps the file locked; repeated loads exhaust handles and block file replacement. +**Fix:** Wrap the handle in a `std::unique_ptr` with a `GDALClose` deleter. + +```suggestion + std::unique_ptr dataset{GDALDataset::Open(path.c_str(), GDAL_OF_VECTOR)}; +``` +```` + +## Constraints + +- No duplicates: never post a finding already covered by an existing PR comment or review, nor the same finding twice within one review. +- Never approve. Review event is always `COMMENT`, never `APPROVE` or `REQUEST_CHANGES`; the human reviewer decides. +- Read-only on code: no edits, commits, pushes or branch checkouts. Do not build or run tests locally; rely on CI checks and reading the code. +- Subagents: use the `GPT-6.1 Sol` model. Delegate only large, genuinely independent parts of the review (for example, a large group of unrelated files). Do not delegate work you can finish in a handful of tool calls, and do not use subagents to re-check your own findings. +- Progress updates: one sentence before starting, then only report when you find something important or change approach. + +## Tools + +Everything goes through `gh` against `MapWindow/MapWinGIS6`. + +| Purpose | Command | +| ------------------- | ------------------------------------------------------- | +| Read linked issue | `gh issue view --comments` | +| Find related issues | `gh issue list --search "" --state all` | +| Find related PRs | `gh pr list --search "" --state all` | +| CI status and logs | `gh pr checks `, `gh run view --log-failed` | + +## Final summary + +After posting the review, reply with a brief summary, leading with the recommendation: + +- Recommendation: Approve | Request Changes +- Risk level: Low | Medium | High, with a one-sentence justification +- Overall assessment: 2-4 sentences +- Top 5 findings: one line each, with severity and `file:line` +- Count of findings per severity +- Count of findings skipped as duplicates of existing comments + +Details live in the PR comments. diff --git a/.idea/.gitignore b/.idea/.gitignore new file mode 100644 index 0000000..f3b42f4 --- /dev/null +++ b/.idea/.gitignore @@ -0,0 +1,12 @@ +# Default ignored files +/shelf/ +/workspace.xml +# Markdown personal settings +/markdown.xml +# Editor-based HTTP Client requests +/httpRequests/ +# Ignored default folder with query files +/queries/ +# Datasource local storage ignored files +/dataSources/ +/dataSources.local.xml diff --git a/.idea/.name b/.idea/.name new file mode 100644 index 0000000..bf99c41 --- /dev/null +++ b/.idea/.name @@ -0,0 +1 @@ +MapWinGIS6 \ No newline at end of file diff --git a/.idea/debug-profiles.local.json b/.idea/debug-profiles.local.json new file mode 100644 index 0000000..f604350 --- /dev/null +++ b/.idea/debug-profiles.local.json @@ -0,0 +1,3 @@ +{ + "debug-profiles": [] +} \ No newline at end of file diff --git a/.idea/github.iml b/.idea/github.iml new file mode 100644 index 0000000..51a1b8b --- /dev/null +++ b/.idea/github.iml @@ -0,0 +1,6 @@ + + + + + + \ No newline at end of file diff --git a/.idea/misc.xml b/.idea/misc.xml new file mode 100644 index 0000000..0b76fe5 --- /dev/null +++ b/.idea/misc.xml @@ -0,0 +1,7 @@ + + + + + + \ No newline at end of file diff --git a/.idea/modules.xml b/.idea/modules.xml new file mode 100644 index 0000000..c476ae7 --- /dev/null +++ b/.idea/modules.xml @@ -0,0 +1,8 @@ + + + + + + + + \ No newline at end of file diff --git a/.idea/vcs.xml b/.idea/vcs.xml new file mode 100644 index 0000000..35eb1dd --- /dev/null +++ b/.idea/vcs.xml @@ -0,0 +1,6 @@ + + + + + + \ No newline at end of file diff --git a/README.md b/README.md index ce59397..a156838 100644 --- a/README.md +++ b/README.md @@ -102,8 +102,15 @@ For additional configurations, dependency features, and troubleshooting, see The CMake build can also include the native `translate` sample and targets for building and running the C# ActiveX test host. Configure with -`-DMWG_BUILD_SAMPLES=ON` to enable them; see the CMake section in the build -guide for target names and commands. +`-DMWG_BUILD_SAMPLES=ON` to enable them: + +```powershell +# Configure with samples and build or launch the C# test host +cmake --preset x64-debug -DMWG_BUILD_SAMPLES=ON +cmake --build --preset x64-debug --target run_activex_host +``` + +See the CMake section in the build guide for target names and additional commands. CI Release artifacts use the `full-drivers` feature. Their tests require HDF5, NetCDF, KML, SpatiaLite, and PostgreSQL support. diff --git a/docs/BUILDING.md b/docs/BUILDING.md index df91671..c06ce97 100644 --- a/docs/BUILDING.md +++ b/docs/BUILDING.md @@ -426,12 +426,15 @@ that inherits from the presets, for example: x86 toolchain. Build `MapWinGIS.Core`, `MapWinGIS.ActiveX`, and `MapWinGIS.Core.Tests` from -CLion's CMake targets. To expose the sample and C# host targets, set -`MWG_BUILD_SAMPLES=ON` in the selected CMake profile. CLion's GoogleTest +CLion's CMake targets. To expose the sample and C# host targets, add +`-DMWG_BUILD_SAMPLES=ON` to the **CMake options** field in the selected profile +(**Settings | Build, Execution, Deployment | CMake**). CLion's GoogleTest integration can run, filter, and debug individual cases. The C# host can be built and launched through `build_activex_host` and `run_activex_host`; -CLion's native debugger does not debug the managed C# code. See -[Testing](TESTING.md#cmake-and-clion) for test-running details. +these targets automatically ensure `MapWinGIS.ActiveX` and runtime data +dependencies are up to date. CLion's native debugger does not debug the +managed C# code. See [Testing](TESTING.md#cmake-and-clion) for test-running +details. ### Using CMake in Visual Studio @@ -475,10 +478,14 @@ Use the short preset build directories under `out\b`. If Windows still rejects long paths, enable the Windows `LongPathsEnabled` policy and restart the terminal before configuring again. -**MIDL cannot find `cl.exe`** +**MIDL cannot find `cl.exe` or CLion fails to detect the Visual Studio toolset** Ninja needs an MSVC environment. Assign the matching Visual Studio toolchain -to the CLion profile, or configure from a VS 2026 Developer PowerShell. Ensure -`cl.exe` is on `PATH` before rerunning CMake. +to the CLion profile, or configure from a VS 2026 Developer PowerShell. If +Visual Studio is installed on a non-standard drive or path (for example, +`D:\Program Files\Microsoft Visual Studio\18\Community`), ensure the Visual +Studio toolchain in **Settings | Build, Execution, Deployment | Toolchains** +points directly to that installation directory rather than relying on automatic +discovery. Ensure `cl.exe` is on `PATH` before rerunning CMake. **ATL headers or libraries are missing** Install **C++ ATL for latest v145 build tools** in Visual Studio Installer for