Skip to content

[fix] Harden installed script copying and reconcile distribution audit - #137

Merged
Chain-Frost merged 279 commits into
mainfrom
fix/pr130-review-and-requirements-20261019
Oct 31, 2026
Merged

Chain-Frost merged 279 commits into
mainfrom
fix/pr130-review-and-requirements-20261019

Conversation

@Chain-Frost

@Chain-Frost Chain-Frost commented Oct 18, 2026 •

Copy link
Copy Markdown
Owner

Purpose and merge plan

Correct the unsafe installed-wrapper copying introduced by #130, reconcile installed scripts and MCP distribution with current main, and retain the original engineering acceptance requirements without claiming blanket sign-off.

Open and unmerged; no auto-merge or publication is authorised. After exact-head CI and explicit maintainer approval, squash merge into main (one commit on main; no history rewrite required on this development branch). No release or automatic destructive operation is authorized by this PR.

Integrated implementation

  • Installed-wrapper safety: exclusive creation without --overwrite, private staged replacement and descriptor-anchored POSIX publication/cleanup, source permission preservation, symlink/swap protections, Windows and POSIX regression tests.
  • Package and lifecycle evidence: 151 tracked ryan-scripts/ source/resource paths = 85 wheel-selected + 66 repository-only; 34 distinct MCP script targets. The AST import-time safety scanner, wheel inventory equality, copied-script smoke and no-checkout install tests protect the provisional selection. copy_file.py and copy_folder.py remain repository-only and are not installed/MCP-exposed.
  • MCP discovery: explicit script origin and external prerequisite/approval classification without implicit execution; optional real GDAL/QGIS test gates.
  • Scoped wrapper fixes: RORB import/CLI behavior, archive ZIP/TAR traversal/link preflight, GDAL raster-to-XYZ staged overwrite, GeoPackage/Shapefile vector-clip preservation and rollback, DOCX argument precedence.
  • QGIS tools from #154: read-only QGS/QGZ source audit (metadata-only existence by default, including directory-backed FileGDB datasets; explicit one-byte cold-storage ping for regular files), bounded read-only QGD inspection, and separate path-repair script writing a new _repaired.qgs/.qgz by default. Configurable UNC/mapped-drive equivalences, safe handling of mapped and unmatched relative layers when output is relocated, protection of unrelated destination QGD files, and editable DEFAULT_* options.
  • Reconciled with current main: merged 820358b through 4f032e9; retained current TUFLOW input preflight, duplicate deletion improvements, package metadata and submodules, and the Python >=3.14 requirement. Supporting #146, #152 and #154 were merged into this PR's branch, not main. #153 (GeoPackage VACUUM) remains closed without merge.

Validation and review

  • Last fully verified code head: 060fc17 — all 6/6 CI checks passed: Python CI and Fast PR. Windows hosted tests: 1,674 passed, 25 skipped, 7 deselected (127 nonfatal warnings, primarily existing deprecations); Policy/strict Pyright: 0 errors, 0 warnings. Package/isolated-wheel, Python 3.14 MCP (legacy status label mentions Python 3.13), POSIX security and Ruff/whitespace passed. The tested code fix head is 837184b; the final commits update only the work record/register and restore an inventory-test-required documentation phrase. The initial Rasterio metadata typing failure at c8d0743 is resolved with an explicit narrow Protocol and read-only regression coverage.
  • Historical successful Windows acceptance: 98 focused MCP tests passed / 10 skipped; bounded long-path, mapped-drive and UNC scratch copying; two real GDAL and two real QGIS checks. Those passes apply to historical tested revisions, not to all current scripts or every network/environment configuration.
  • All 22 [fix] Harden installed script copying and reconcile distribution audit #137 inline review threads resolved, plus 4/4 resolved on integrated [qgis] Audit QGS/QGZ cold-storage sources, repair paths and inspect QGD #154. The consolidated Codex review at 350d41b identified two QGIS P2 issues: unmatched relative sources on relocated outputs and directory-backed datasets in existence audits. Both were fixed and regression-tested at 837184b, retested on final head 060fc17, and the threads resolved with exact-head evidence. The separately detected QGS/QGD companion collision was fixed (d181b15, with regression coverage). No additional open inline findings were present at this final status.
  • Inventory and requirements are linked below. No production data was used in the synthetic hosted tests.
  • 1 November 2026 acceptance update: documentation head fa20397 records AB-023 closure and the grouped AB backlog. The previous executable-code head 060fc17 passed all six CI jobs. The documentation-only head's Fast PR checks passed; Python CI is queued. The PR remains unmerged; there is no package release.

Explicitly open acceptance — not silently waived

  • Original A–F requirements and current workstream statuses remain authoritative. They distinguish bounded implementation from engineering acceptance.
  • 151-file lifecycle/ownership inventory and 47-ID remediation/acceptance backlog are follow-up records: 46 open/deferred/partially verified; HY-8 AB-023 explicitly accepted and closed on 1 November 2026. These are not 46 proven defects or 151 individually approved executable scripts.
  • Still open only where recorded: native TUFLOW launcher, project initialiser, ASC_to_ASC and controlled-model/failure acceptance (AB-012/013/029); workflow-specific Windows/UNC/locked-file and interruption cases; specific 7z/RAR, GIS/Office, RORB and CLI/output contracts; and proportionate legacy/maintained lifecycle decisions. HY-8 numerical and wrapper acceptance is closed by maintainer decision (AB-023) based on already-accepted ryan-culverts PR #29. Do not rerun existing accepted comparisons or broaden the gate to historical/non-executable resources.
  • These open validations must be explicitly assigned, accepted or retained as gates for affected script promotion/engineering use and publication. They are not a claim that every deferred item must be completed in this corrective code PR.

Before squash merge

  1. Code verification complete: six checks green at 060fc17, including strict Pyright, QGIS regressions, isolated-wheel, inventory and security; 151/85/66/34 counts and unsafe-wrapper exclusion verified. Verify the newer documentation-only head separately.
  2. Complete: integrated code/safety review, including fresh Codex feedback. All 22 [fix] Harden installed script copying and reconcile distribution audit #137 threads resolved, fixing both new P2 issues with exact-head tests.
  3. Accepted: HY-8 comparison evidence, with AB-023 closed. Remaining: retain the explicitly scoped TUFLOW, Windows/UNC and other AB work as future maintenance/engineering tasks; passing CI is not a blanket engineering release certificate.
  4. Maintainer approval outstanding: authorize the later squash merge to main. Do not merge, auto-merge or publish automatically.

The branch contains historical implementation commits for traceability; the eventual merge is intended to produce a single commit on main.

@Chain-Frost
Chain-Frost marked this pull request as ready for review October 18, 2026 18:56
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-31T16:34:38.590811Z 060fc17 Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d685c57362

ℹ️ 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 thread ryan_library/mcp/installed_scripts.py Resolved

Copy link
Copy Markdown
Owner Author

@codex review

Please review the current head dd4db090bae759327a965d069fc0d21368f5d765 after the P2 permissions fix, MCP provenance/external-prerequisite diagnostics, git-tracked static inventory script, and representative fully provisioned wrapper help tests. All five CI checks passed at this head. In particular, review atomic copy race/permissions behavior, honest limits of runtime readiness, and new inventory script type/runtime safety. Do not treat existing unverified original A–F requirements as closed merely because this corrective PR passes CI.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dd4db090ba

ℹ️ 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 thread ryan_library/mcp/installed_scripts.py Resolved
Comment thread ryan_library/mcp/installed_scripts.py Resolved
Comment thread docs/MCP_SETUP.md Resolved

Copy link
Copy Markdown
Owner Author

@codex review

Please review latest commit 597d941f90b4d4c949076e24f55eee3536a38278. All prior inline threads have now been addressed/resolved after all five checks passed. Verify the private staging-directory race protections, POSIX descriptor-based chmod, Windows read-only new-copy behavior, and source packaging documentation. Do not assume that passing CI establishes completion of the separately tracked original A–F acceptance criteria.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 597d941f90

ℹ️ 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 thread ryan_library/mcp/installed_scripts.py Resolved

Copy link
Copy Markdown
Owner Author

@codex review

Please review exact current head 3608408eb827b5cb4326c76ad8fd5997b01b9fd3, particularly the fd-anchored POSIX staging publication and stage-directory rename regression. The previously identified five inline review threads are now resolved and all five CI checks pass at this head. Please flag any remaining real security or portability issues. Do not interpret this as approval to merge or as completion of the separate original A–F acceptance work.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3608408eb8

ℹ️ 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 thread ryan_library/mcp/installed_scripts.py Resolved

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 350d41bacc

ℹ️ 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 thread ryan_library/functions/qgis_project.py Resolved
Comment thread ryan_library/functions/qgis_project.py Resolved

Copy link
Copy Markdown
Owner Author

31 October 2026 — integrated PR #137 exact-head validation and review handoff

Code head: 350d41b; all six checks green:

  • Python CI: Policy / strict Pyright 0 errors, 0 warnings; Package / isolated-wheel, Python 3.14 MCP and POSIX wrapper-security checks passed. The old GitHub job title still mentions Python 3.13, but runs Python 3.14.
  • Fast PR: Ruff and whitespace passed.
  • Hosted Windows tests: 1,670 passed, 25 skipped, 7 deselected, 127 nonfatal warnings. Most warnings are existing vendored numerical, Pandas deprecation and affine-operation warnings; do not interpret them as broader app acceptance.

Changes in this final pass:

  • Fixed the 11 Rasterio attribute typing errors introduced by updated third-party type inference using a narrow metadata protocol; added a read-only output regression and missing-file check.
  • Identified and fixed a further QGS repair collision: a pre-existing destination .qgd now prevents generating a repaired .qgs, even when the input has no companion QGD. Regression verifies refusal and preservation of the unrelated auxiliary bytes.
  • Reconciled the lifecycle ledger and verified 151 unique source/resource entries = 85 wheel-selected + 66 repository-only. Both unguarded copy_file.py and copy_folder.py remain excluded. The 34 MCP script targets are separately tracked.
  • Integrated PR #154 into [fix] Harden installed script copying and reconcile distribution audit #137 (not main) with its four resolved threads; [fix] Harden installed script copying and reconcile distribution audit #137's earlier 20 inline threads are all resolved. The new QGD issue above was fixed on this head.

Review boundary: Focused integrated inspection covered the wrapper-copy publication/cleanup, archive preflight, GDAL overwrite/rollback, QGIS audit/repair and auxiliary SQLite safeguards, package inventory, MCP exposure and regression evidence. No additional confirmed source defect remains from that inspection, but it is not individual script lifecycle or engineering output approval. A new Codex review was requested on the integrated head; its response is outstanding and any new findings must be handled before approving merge.

The 47-entry script remediation/acceptance backlog and original requirements preserve open owner/lifecycle, licensed TUFLOW/HY-8, real 7z/RAR, UNC/mapped-drive and application-specific output/interruption gates. Those are not automatically waived by CI. No production data, release publication or automatic destructive execution was used.

Disposition: Leave #137 draft, open, unmerged until the fresh review and explicit maintainer decision. The later approved delivery is a squash merge to main, yielding one commit on main. No merge or auto-merge was performed.

Copy link
Copy Markdown
Owner Author

Final integrated PR #137 review and CI closure — 31 October 2026

Exact final PR head: 060fc17. All six checks passed at this SHA:

The two final Codex P2 findings were fixed, functionally tested and explicitly acknowledged in their threads:

  1. QGIS relocated outputs: an unmatched local relative layer is rebased against the repaired project's output directory even when a different layer matches --replace-root; QGS/QGZ tests prove mapped/unmatched/absolute inputs still point to the correct datasets without altering source bytes.
  2. Directory-backed GIS sources: metadata-only existence audits now accept a FileGDB directory without reading its contents; explicit cold-storage pings report that a directory cannot be byte-read as a regular file. Library and wrapper regressions cover both modes.

Earlier final inspection also added a safeguard against a repaired QGS picking up an unrelated existing destination QGD file. The Rasterio metadata type issue was corrected without suppressing strict Pyright. The 151 tracked = 85 installed + 66 repository-only, 34 MCP targets inventory is consistent. The deliberately unsafe unguarded copy-file/folder scripts remain excluded from installation.

The work record/register now link the tested code head and the still-open acceptance gate. The 47-item remediation/acceptance list includes future lifecycle decisions and application-specific validation, not 47 outstanding confirmed code defects. Representative TUFLOW/HY-8, GDAL/QGIS/native Windows/UNC/7-Zip/interrupt recovery, numerical parity and individual script lifecycle approval remain open where specifically documented. No production engineering, distribution-release or external-app blanket acceptance is claimed.

Delivery: PR #137 remains open, draft and unmerged, with no auto-merge or publication. No further technical CI or inline review blockers are currently identified. Obtain explicit maintainer disposition of residual engineering/lifecycle gates and approval before the later squash merge into main, which will produce one commit on main.

@Chain-Frost
Chain-Frost marked this pull request as ready for review October 31, 2026 16:32

Copy link
Copy Markdown
Owner Author

1 November 2026 — maintainer decision on external acceptance and AB backlog

Recorded in the durable Markdown AB backlog and requirements/status record:

  • HY-8 AB-023: CLOSED by explicit maintainer acceptance decision. Existing numerical comparison evidence from merged ryan-culverts PR #29 is accepted. No repeat numerical-baseline or additional HY-8-wrapper acceptance campaign is required to deliver [fix] Harden installed script copying and reconcile distribution audit #137. This does not claim the 22 October Windows [mcp] Validate curated workflows and Windows engineering acceptance for #137 #146 acceptance run executed HY-8.
  • Native TUFLOW remains OPEN, specifically the launcher (AB-012), initializer (AB-013), ASC_to_ASC (AB-029), and the separate model preflight/TLF input checks (AB-046/047). Validate controlled licensed model execution, exits, interruption, data preservation and outputs only as relevant to each script.
  • Windows/UNC remains scoped to affected workflows. Prior authorised mapped/UNC copying, long-path and real GDAL/QGIS checks are accepted; no generic rerun. Test locked-file, network and interruption cases only when an AB item records an unverified path.
  • 47 historical AB IDs are retained: 46 remain open/deferred/partially verified, 1 (AB-023) is accepted-closed. The backlog now separates active installed-workflow safety, external/engineering acceptance, historical/repository-only lifecycle decisions, cross-cutting inventory/contracts and accepted closure. Historical or non-executable files do not require blanket full engineering validation simply by being retained in Git.

The supporting Windows acceptance record and affected script inventory entries have been reconciled. The decisions modify documentation/acceptance scope only; no production scripts or simulation data were changed. The previous tested code head 060fc17 passed all six checks. The new documentation head requires its own CI evidence.

PR #137 remains open/unmerged for a later squash merge to main when approved; no merge or publication was performed.

@Chain-Frost Chain-Frost self-assigned this Oct 31, 2026
@Chain-Frost
Chain-Frost merged commit 6e2d0f1 into main Oct 31, 2026
6 checks passed
@Chain-Frost
Chain-Frost deleted the fix/pr130-review-and-requirements-20261019 branch October 31, 2026 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant