Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions .github/skills/implementing-plan-steps/SKILL.md
Original file line number Diff line number Diff line change
@@ -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
---
Expand Down Expand Up @@ -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 <branch> .worktrees/<branch> origin/development
git fetch origin <base-branch>
git worktree add -b <branch> .worktrees/<branch> origin/<base-branch>
```

`<branch>`: short kebab-case name derived from the step. Work only inside the worktree from here on.
`<base-branch>` is `development` when there is no open previous-phase PR, or
the head branch of that open PR. `<branch>`: 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.

Expand All @@ -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.
Expand All @@ -85,7 +94,7 @@ Compare warnings with the latest known `Debug | x64` baseline for the same toolc

```bash
git push -u origin <branch>
gh pr create --base development --title "<plan title>" --body-file <temporary-body-file>
gh pr create --base <base-branch> --title "<plan title>" --body-file <temporary-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.
Expand Down
188 changes: 188 additions & 0 deletions .github/skills/reviewing-pull-requests/SKILL.md
Original file line number Diff line number Diff line change
@@ -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: "<PR number or URL> [base branch]"
---

# PR review

## Inputs

- PR: a number resolves to `https://github.com/MapWindow/MapWinGIS6/pull/<number>`; 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 <n> --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 <issue> --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 <base>
git show origin/<base>:AGENTS.md
git ls-tree --name-only origin/<base> docs/
```

**2. Fetch diff, PR head and existing comments**

```bash
gh pr diff <n> --repo MapWindow/MapWinGIS6
git fetch origin +pull/<n>/head:pr-<n>
git show pr-<n>:<path> # full file at PR head
gh api repos/MapWindow/MapWinGIS6/pulls/<n>/comments --paginate --jq '.[] | {path, line, original_line, body}'
gh api repos/MapWindow/MapWinGIS6/pulls/<n>/reviews --paginate --jq '.[] | {state, body}'
gh api repos/MapWindow/MapWinGIS6/issues/<n>/comments --paginate --jq '.[] | .body'
gh pr checks <n> --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/<n>/comments/<comment-id>/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": "<headRefOid>",
"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/<n>/reviews --method POST --input <tempfile>
```

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/<n>/reviews/<review-id>/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<GDALDataset, GdalDatasetCloser> 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 <issue> --comments` |
| Find related issues | `gh issue list --search "<text>" --state all` |
| Find related PRs | `gh pr list --search "<text>" --state all` |
| CI status and logs | `gh pr checks <n>`, `gh run view <run-id> --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.
12 changes: 12 additions & 0 deletions .idea/.gitignore

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions .idea/.name

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions .idea/debug-profiles.local.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions .idea/github.iml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions .idea/misc.xml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions .idea/modules.xml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions .idea/vcs.xml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 9 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
21 changes: 14 additions & 7 deletions docs/BUILDING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down