feat(migration): server-attested toolbox translation verdicts - #194
Conversation
The arcpy/toolbox codemod classified every tool from the SDK's own view of the Honua process catalog. That view can drift from the server that would actually run the job, so a migration report could call a tool translated when the submit validator would reject it, or flag one unsupported when the server would accept it. honua-server#3040 landed the server side of this (POST /api/v1/admin/import/toolbox/translation/validate, honua-server#2145): it validates a translated toolbox manifest against the canonical process catalog and returns a per-tool translated / partially-translated / unsupported classification with the reasons a tool cannot be fully translated. This wires the SDK to it. - honua_admin: typed manifest/report models plus HonuaAdminClient.validate_toolbox_translation (sync + async), on the existing admin credential path since the endpoint is in the admin import group. No new auth mechanism. - honua_sdk.migration.attestation: builds the manifest from a parsed .pyt / .atbx toolbox and merges a server verdict over the local one. The server wins on disagreement and the disagreement is reported rather than silently overwritten; a local verdict is never presented as attested. Offline, unreachable, unauthorized, and malformed-response paths all degrade the whole report to an explicitly marked local-only verdict with a stated reason. There is no partial attestation. The validator is injected, so the merge logic stays pure and offline. - honua-migrate: --server / --api-key / --attestation / --require-attested on translate, pyt, and atbx. translate now also accepts a .pyt/.atbx toolbox; on a bare arcpy .py script it refuses --server rather than inventing a toolbox sourceFormat the endpoint would reject. - Binary .tbx stays a policy refusal, never a parser. The error now carries the concrete ArcGIS Pro export steps that produce a readable .atbx/.pyt, so it reads as a migration instruction instead of a dead end. - resolve_argument_bindings exposes the source-argument to canonical- parameter pairing the flattened OGC payload had discarded; _translate_call now shares that one resolution instead of duplicating it. Closes #188
![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: 04a3460478
ℹ️ 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".
|
CI note: This PR touches |
) All three let a report claim `attested: true` while the verdict was not, in fact, fully attested -- the exact failure the issue's acceptance criteria forbid. .atbx script tools were missing from the manifest. parse_atbx_toolbox records them in script_tool_names because their logic lives in an external .py the reader deliberately does not follow, but the manifest was built from toolbox.models alone. The server then returned a clean report for a strict subset of the toolbox and the CLI presented it as whole-toolbox attestation. Script tools are now submitted with no proposed target -- the honest statement, since the translator never read the body -- so the report's tool count matches the toolbox and they come back explicitly unsupported. The admin response model synthesized artifact identity. from_dict defaulted a missing artifactKind/artifactVersion to the expected values, and the CLI serializes the model back to a dict before attest_translation validates it, so the attestation layer could not tell an error envelope from a genuine v1 report. Both fields are now `str | None`, default None, never filled in client-side, and to_dict round-trips their absence. _parse_report now REQUIRES both -- the genuine endpoint always stamps them -- instead of accepting a missing identity. Classifications outside the vocabulary were accepted. A value such as `manual-review` or `translated-v2` became a tool's effective classification while no summary counter tallied it, so the attested report did not add up. Classifications are validated against the three declared values and anything else degrades to local-only. Adds regression tests for each: an .atbx holding both a model and a script tool, a 200 missing artifactKind/artifactVersion (both directly and through the admin model round-trip), and out-of-vocabulary classifications -- each asserting the result is NOT falsely attested. Also pins the invariant the vocabulary check protects: an attested summary accounts for every tool. Related to #188
|
@codex review All three review findings are fixed in
Please re-check specifically that no path can still reach Note the branch is level with |
GitHub Actions
Failure
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3da5b61f20
ℹ️ 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".
Same false-attestation class as the .atbx script-tool gap, on the other reader. A .pyt declares its tools as `self.tools = [A, B]`; a name whose class is not defined in that file -- imported from another module -- stays in `declared_tool_names` but never materialises into `toolbox.tools`, because there is no execute() body to classify. The manifest was built from `toolbox.tools` alone, so the server returned a clean report covering only the locally-defined tools while the CLI presented it as whole-toolbox attestation. Declared names absent from the materialised tools are now submitted with no proposed target -- nothing was read, so nothing is claimed -- and come back `unsupported`, with a construct pointing at the arcpy .py scanner as the way to classify them properly. Both readers now share one `_unresolved_tool_proposals` helper, since .atbx script tools and imported .pyt tool classes are the same shape: discovered by name, with no body available. Tests: a mixed toolbox (one local tool, one imported), the end-to-end attestation over it asserting the report covers both and its summary adds up, and the degenerate all-imported toolbox that previously produced an empty manifest the endpoint would reject outright. Related to #188
|
@codex review Fourth finding fixed in The invariant across all four findings, stated once so it is easy to re-check: Worth a specific look at whether any other reader surfaces a tool the manifest builder does not submit. I have covered |
GitHub Actions
Failure
…#188) Third instance of the same false-attestation class, found while checking whether the .pyt and .atbx fixes had missed a sibling. parse_atbx_toolbox deliberately keeps a stepless model out of `models` -- there is nothing to translate -- but it also dropped the name entirely, so the tool was invisible to the manifest builder and the server could return a clean report for a toolbox that declared more tools than were ever submitted. ModelBuilderToolbox gains `unresolved_tool_names` (surfaced in to_dict as `unresolvedToolNames`) holding those declared-but-unresolvable names. The `models` contract is unchanged -- a stepless model is still not a model -- but the name is now discoverable rather than lost, and the manifest submits it with no proposed target. That makes all three .atbx tool kinds -- models, script tools, and unresolved models -- go into the manifest, so the report's tool count matches the toolbox. Tests: the stepless-model case end to end, an .atbx exercising all three kinds at once, and the pre-existing test_atbx_model_detected_by_tool_type tightened to pin that the dropped name is now recorded instead of lost. Related to #188
|
Answering my own question above: there was a third instance, and I found and fixed it in
All three I checked the remaining reader surfaces for the same shape and did not find another:
Happy to be proven wrong on that sweep — that is exactly what I would rather hear before this lands than after. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6eb0c01a4d
ℹ️ 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".
GitHub Actions
Failure
… about (#188) _parse_report checked artifact identity and tool names, but never that the report was about the artifact just submitted. Two toolboxes can share tool names, so a stale or misrouted 200 -- a caching proxy, a validator answering for a different toolbox -- would have had its classifications accepted and the wrong artifact marked attested. The endpoint echoes toolboxName and sourceFormat back for exactly this reason. Both are now compared against the submitted batch before the tools array is trusted, tolerating the server's own normalisation (it Trim()s the name and lower-cases the format). A mismatch or a missing field degrades the report to local-only like any other untrustworthy response. Tests: wrong/missing toolboxName and wrong/missing sourceFormat each assert local-only, plus a case proving the server's own trimming and lower-casing still binds. The shared malformed-report fixtures now carry a valid envelope so each case still exercises the specific check it names rather than tripping the new binding check first. Related to #188
|
Both threads from the latest review are addressed and resolved.
Running total across this review cycle — six findings, all in the same family, all fixed with regression tests:
The invariant they all serve, now enforced in one place: Local gates on The only red check is |
GitHub Actions
Failure
1 participant
Closes #188
Summary
The arcpy/toolbox codemod classified every tool from the SDK's own view of the Honua process catalog. That view can drift from the server that would actually run the job, so a migration report could call a tool translated when the submit validator would reject it, or flag one unsupported when the server would accept it — and migration reports are used to decide whether a migration is viable.
honua-server#3040 landed the server side (
POST /api/v1/admin/import/toolbox/translation/validate, honua-server#2145). It validates a translated toolbox manifest against the canonical process catalog and returns a per-tooltranslated/partially-translated/unsupportedclassification with the specific reasons a tool cannot be fully translated. This PR wires the SDK to it, so a toolbox verdict can be server-attested instead of an SDK-local assertion.Two rules govern the result:
local-onlywith an explicitfallbackReason. Unreachable server, refused credentials, a malformed or incomplete response — all degrade the whole report. There is no partial attestation.Changes Made
honua_admin— typedToolboxTranslationManifest/ToolboxTranslationReport(+ descriptor, mapping, binding, issue, summary models) andvalidate_toolbox_translationon bothHonuaAdminClientandAsyncHonuaAdminClient. The endpoint is in the admin import group, so it rides the existing admin credential path (api_key/auth_provider) — no new auth mechanism. Sync client regenerated withscripts/gen_sync.py.honua_sdk.migration.attestation(new) — builds the wire manifest from a parsed.pyt/.atbxtoolbox (build_pyt_translation_manifest/build_atbx_translation_manifest) and merges a server verdict over the local one (attest_translation). The validator is injected, so the merge logic itself is pure, offline, and carries no dependency on the admin client. Manifests larger than the endpoint's 200-tool cap are submitted in batches and merged, rather than truncated or rejected.honua-migrate—--server,--api-key(or$HONUA_ADMIN_API_KEY),--attest-timeout,--attestation,--require-attestedontranslate,pyt, andatbx.translatenow also accepts a.pyt/.atbxtoolbox.honua-adminis imported lazily and stays optional: without it the toolbox still translates and the report is simply markedlocal-only..tbxrefusal made actionable — still never parsed (proprietary container; export-to-open-format is the standing rule, same as.loc/.lox), but the error now carries the concrete ArcGIS Pro export steps that produce a readable.atbx/.pyt, so it reads as a migration instruction rather than a dead end. Shared by both thepytandmodelbuilderreaders.resolve_argument_bindings— exposes the source-argument to canonical-parameter pairing that the flattened OGC payload dict had discarded._translate_callnow shares that single resolution instead of duplicating it, and the dead_assign_process_valuehelper is gone.docs/honua-gp/codemod-translation-coverage.mdgains a "Server-attested verdicts" section; README CLI line updated.compatibility/public-api.jsonregenerated for the newhonua_adminsurface.Scope note
Attestation is toolbox-scoped because the endpoint's manifest declares a toolbox
sourceFormat(pyt/atbx/tbx).honua-migrate translateon a bare arcpy.pyscript therefore refuses--serverwith an actionable message rather than inventing a format the server would reject; pointtranslateat a.pyt/.atbxtoolbox for an attested verdict.Finding surfaced by this work
The SDK's proposed target parameter names already differ from the canonical catalog for at least
geometry.buffer(SDK proposesinput_features/distance; the server's shared fixture useswkb/srid/distance). That drift is exactly what this feature is for — it now shows up asunknown-target-parameter/missing-required-parameterissues and a reported disagreement instead of a confidently wrong local verdict. Reconciling the registry against the live catalog is deliberately not in this PR: the point of the change is that the server is authoritative, and a follow-up should be driven by a real attestation run rather than by guessing.Explicitly not done
.tbxparsing.UnsupportedToolboxErroris a policy decision, not an unimplemented stub, and was not "fixed".tests/test_custom_code_batch_only_policy.pytripwire still passes).Breaking Changes
None.
honua_admingains new exports and a new method;honua_sdk.migrationgains new exports. Existing signatures and report shapes are unchanged, with two additive keys on the toolbox commands' emitted document (attestation, andtranslationManifestontranslate).Testing
Run locally against Python 3.12 with both packages installed editable (
grpc,geopandasextras):ruff check .— All checks passedpython scripts/gen_sync.py --check— Generated sync files are up to date; sync/async twins are in locksteppython -m mypy packages/honua-sdk/honua_sdk packages/honua-admin/honua_admin— Success: no issues found in 70 source filespython -m pytest tests/ -q --cov=honua_sdk --cov=honua_admin --cov-fail-under=94— 1576 passed, 18 skipped; total coverage 94.36% (gate 94)python -m pytest tests/ -q --cov=honua_sdk --cov-fail-under=93— 94.09%python -m pytest tests/admin -q --cov=honua_admin --cov-fail-under=93— 94.71%python scripts/compatibility_gate.py— Compatibility gate passedpython scripts/gen_sdk_coverage.py— SDK coverage gate passedNew tests (43 added) cover the four paths the issue names plus the offline default:
local-onlywith the transport failure as the reason, local verdicts intactattested, noserverClassificationinvented, and--require-attestedexits non-zerosourceLabel, unique tool names for multi-step tools, output arguments excluded from parameter mappings, unmapped keywords reported as unsupported constructshonua-adminabsent → still translates, markedlocal-only.tbxrefusal carrying its export instructions