fix(example): reconcile worked example validation - #18
Merged
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdd2117361
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+1557
to
+1561
| ("candidates_tier1", True, [("candidate_parcels_layer", "Tier 1 Candidate Parcels", "ogr", True)]), | ||
| ("candidates_tier2", True, [("candidate_parcels_tier2_layer", "Tier 2 Candidate Parcels", "ogr", True)]), | ||
| ("candidates_highway", False, [("candidate_parcels_tier3_layer", "Tier 3 Highway-only Candidate Parcels", "ogr", True)]), | ||
| ("catchments", True, [("education_catchments_layer", "Education 2 km Straight-line Proxies", "ogr", True)]), | ||
| ("education_pois", True, [("education_pois_layer", "Active Municipal Schools & Kindergartens", "ogr", True)]), |
There was a problem hiding this comment.
When the generated or committed QGIS project is opened, this sequence places candidate polygons first, followed by catchments, POIs, overrides, and infrastructure—the same bottom-to-top order used by the web map. QGIS paints its first tree entry on top, so the polygon layers obscure or tint the POIs, roads, and scenario features that should render above them; the PyQGIS branch constructs the same incorrect order. Reverse the groups and the layers within each group while retaining the basemap last so the canonical worked example mirrors the dashboard.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
| group_node = ET.SubElement(layer_tree, "layer-tree-group", { | ||
| "name": group_titles[group_id], | ||
| "expanded": "1" if expanded else "0", | ||
| "checked": "Qt.Checked", |
There was a problem hiding this comment.
For candidates_highway, whose manifest declares default_open: false, the dashboard initializes the corresponding layer group as hidden, but this generator uses that flag only for expansion and hardcodes every QGIS group as checked. Consequently the committed QGIS view opens with Tier 3 visible while the web view hides it; the PyQGIS branch likewise only collapses the group without disabling its visibility. Derive the checked state from default_open in both generation paths.
AGENTS.md reference: AGENTS.md:L29-L29
Useful? React with 👍 / 👎.
`openmapstack verify` reported the worked example as broken in ways the example was not, and passed over one way it was: - `groups_match_manifest` compared a tree name read straight out of the .qgs against the manifest's raw title, so a group titled "Schools & Kindergartens" arrived as "Schools & Kindergartens" and could never match. Unescape before folding. - `layers_match_manifest` demanded the .qgz open the exact format variant the manifest layer names. A web map reads final-candidates.json while its QGIS companion opens the .gpkg written by the same step; they are one layer in two formats, and the check forced the desktop project onto the weaker file to satisfy a string match. Accept format siblings, the declared one first. - The same check failed a layer the manifest declares as browser-local draft state, which by definition has no file and cannot be in a .qgz. Layers marked `persistence: local_storage` are skipped (and counted in the pass message) rather than failed. Also corrects `runtime_load`'s docstring, which claimed it asserts on layer groups; `groups_match_manifest` is what does. Refs #14 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016p2qykgd7ZnaA6LnkyXCEj
The worked example shipped a run record whose input inventory no longer matched pipeline.py: the pipeline was edited and never re-run. CI never saw it, because CI validates that example with --preflight, and preflight skips the run-record check entirely. It skips it for a real reason -- the full check needs the generated outputs, and a project that gitignores its data has none in a fresh clone. But the inputs that *are* committed sit right there, and the pipeline is exactly where a record goes stale. `runs.present_files` compares every run-record entry whose file exists in the checkout against its recorded hash, and says nothing at all when there is no record yet, which is the normal state preflight exists for. Refs #14 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016p2qykgd7ZnaA6LnkyXCEj
`examples/tartu-development` is the artifact the skill points agents at, and it failed its own validator. Three separate faults, one cause: the QGIS project was maintained by hand beside the manifest rather than generated from it. - **Layer groups.** The manifest declared 7 semantic groups; the .qgz organised the same layers into 4 thematic ones, so the desktop project showed a different structure than the manifest promised (`qgis.groups_match_manifest`). The tree is now generated from `presentation.map.layer_groups` / `.layers`: each suitability tier is its own layer, filtered by an OGR subset, in its own group, mirroring the three toggles the dashboard offers. A manifest group with no QGIS counterpart now fails the run instead of shipping. - **Coordinate reference systems.** Every `<srs>` carried only `<srid>`/`<authid>`, which QGIS reads back as an *invalid* CRS. Every check passed -- the layers were valid, the authids were right, the render was not blank -- while QGIS could build no transform and painted nothing for any layer whose CRS differed from the map's. In the shipped project that was every layer except the candidate parcels. The full WKT-bearing blocks are now emitted, plus the `ProjectionsEnabled` property without which QGIS discards `<projectCrs>` on read. `qgis.every_declared_layer_renders` goes from failing to passing. - **Basemap.** The default background was CARTO's raster XYZ Positron, which now answers unauthenticated requests with an "API KEY REQUIRED" watermark -- printed across every QGIS view of the analysis. QGIS gets the Maa- ja Ruumiamet Baaskaart WMS instead: authoritative, key-free, native EPSG:3301. The manifest also claimed the dashboard loads those raster tiles; it loads the CARTO Positron *vector* style, and now says so. The run record is regenerated, so the inventory hash matches pipeline.py again. The pipeline's own `qgis_project_static_valid` check now asserts the manifest's groups are in the tree and that the tier subsets cover every tier the data carries, so this drift cannot recur silently. `openmapstack validate` and `openmapstack verify` both come back clean (warning status, from the two documented and honest warnings), and all seven QGIS checks pass under PyQGIS. Closes #14 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016p2qykgd7ZnaA6LnkyXCEj
…bled The guidance taught the `<srs><spatialrefsys><srid>…<authid>…</srs>` form that produced the worked example's silently unrendered layers: QGIS reads that back as an invalid CRS, so `layer.crs().authid()` still answers correctly, every static check passes, and any layer whose CRS differs from the map's paints nothing. Both reference documents now require the full element and the `ProjectionsEnabled` project property, and note that CARTO's raster basemap tiles are watermarked without an API key while their vector styles are not. The eval fixture's reference project gains the same `<projectCrs>` and property, so the artifact the evals hold up as correct follows the rule the documents state. Refs #14 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016p2qykgd7ZnaA6LnkyXCEj
`evals.yml` smoke-tested the example with `--preflight`, which skips the output, report and run-record checks -- so the example had never been fully validated in CI, and shipped a stale run record. It has to stay preflight there: the example's sources and derived outputs are gitignored, and a fresh checkout has nothing for the artifact checks to read. What preflight now does catch is the drift that shipped, via `runs.present_files`. The complete validation moves to its own workflow, which regenerates the data from the real Estonian services first and then runs `validate`, `verify`, and a check that the committed project.qgz is still the one pipeline.py produces. It runs weekly, on demand, and on PRs that touch the example or the package -- not on every PR, so an upstream outage cannot redden unrelated work. Refs #14 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016p2qykgd7ZnaA6LnkyXCEj
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.
fixes #14