Skip to content

Port to Rust#49

Draft
jpage-godaddy wants to merge 2 commits into
godaddy:mainfrom
jpage-godaddy:rust-port
Draft

Port to Rust#49
jpage-godaddy wants to merge 2 commits into
godaddy:mainfrom
jpage-godaddy:rust-port

Conversation

@jpage-godaddy
Copy link
Copy Markdown
Collaborator

Port code to rust, based on cli_engine

Port code to rust, based on `cli_engine`
@github-advanced-security
Copy link
Copy Markdown

You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool.

What Enabling Code Scanning Means:

  • The 'Security' tab will display more code scanning analysis results (e.g., for the default branch).
  • Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results.
  • You will be able to see the analysis results for the pull request's branch on this overview once the scans have completed and the checks have passed.

For more information about GitHub Code Scanning, check out the documentation.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

@wcole1-godaddy
Copy link
Copy Markdown
Contributor

PR #49 Rust port review: broken/missing feature parity

I reviewed this PR against origin/main and also read godaddy/cli-engine since this port is based on it. I am not treating application command syntax changes as bugs by themselves. If the new surface is intentionally --name, --id, --application-id, etc., that is fine as long as equivalent capability/workflows exist.

However, there are still many missing/broken features. I would block this PR until these are resolved or explicitly scoped as intentional breaking changes with a migration plan.


Critical blockers

1. API catalog $ref dereferencing is broken

The committed Rust API catalog contains unresolved external $refs throughout the generated JSON. The current main catalog has zero external refs in generated API JSON; this PR has hundreds.

I walked rust/schemas/api/*.json and found:

  • 1071 total $refs
  • 587 external $refs
  • 587 missing external $refs

Examples include:

  • ./common-types/link-description.json
  • ./link-description.json
  • ./error-details.json
  • ./common-types/v1/schemas/json/error.json
  • ./common-types/v1/schemas/json/uuid.json
  • ./models/Order.yaml
  • ./types/Id.yaml
  • ./types/Uuid.yaml

This matches the reported links.$ref regression. The generator tries to dereference, but unresolved refs are left in output when external files are unavailable.

Evidence:

  • Fallback preserves original $ref: rust/tools/generate-api-catalog/src/main.rs:583-586
  • Missing ref returns None: rust/tools/generate-api-catalog/src/main.rs:652-665
  • api describe returns raw schemas, so these broken refs are user-visible: rust/src/api_explorer/mod.rs:274-285

Impact: godaddy api describe returns broken schema references instead of usable schemas.


2. Bare godaddy no longer returns JSON discovery

The current GoDaddy CLI root returns JSON with command tree, environment/auth snapshot, and next actions. cli-engine returns rendered clap long help text for empty command paths.

Evidence:

  • cli-engine/src/cli.rs:830-834
rendered: if command_path.is_empty() {
    self.root.clone().render_long_help().to_string()
}

This is a major agent-first discovery regression unless the port adds a custom root command that restores the old JSON discovery behavior.


3. Envelope shape changed from the documented GoDaddy CLI contract

Old/current contract:

{
  "ok": true,
  "command": "godaddy env get",
  "result": {},
  "next_actions": []
}

cli-engine contract:

{
  "data": {},
  "metadata": {},
  "error": null,
  "warnings": [],
  "next_actions": []
}

Evidence:

  • cli-engine/src/output/envelope.rs:9-28

Every consumer checking ok / result breaks unless this is an accepted breaking contract change and the README/skills/docs are updated accordingly.


4. Error JSON goes to stderr, not stdout

The current GoDaddy CLI contract says stdout is always the JSON envelope; stderr is diagnostics only. cli-engine writes rendered output to stdout only on exit 0; errors go to stderr.

Evidence:

  • cli-engine/src/cli.rs:600-604
if output.exit_code == 0 {
    stdout.write_all(output.rendered.as_bytes())?;
} else {
    stderr.write_all(output.rendered.as_bytes())?;
}

Agents parsing stdout will see empty output on failures.


5. --output was reintroduced and --pretty is missing

The current GoDaddy CLI explicitly removed --output; JSON is the only executable output format. cli-engine adds:

  • --output
  • --json
  • --toon
  • --human

Evidence:

  • cli-engine/src/flags.rs:59-170

The PR README still says --pretty is supported, but cli-engine has no --pretty; JSON is always pretty-rendered.

Evidence:

  • No pretty global flag in cli-engine/src/flags.rs:59-170
  • JSON is always rendered with serde_json::to_string_pretty: cli-engine/src/output/json.rs:5-14

So godaddy --pretty ... should fail despite the README saying it works.


6. Streaming contract is not equivalent

Current deploy streaming contract: NDJSON progress lines, then terminal result or error envelope as the final line.

cli-engine streaming writes handler events directly, then on success returns no terminal rendered output.

Evidence:

  • cli-engine/src/cli.rs:1772-1835
  • Successful stream returns rendered: String::new(): cli-engine/src/cli.rs:1827-1831

Rust application deploy sends step/progress events but no terminal result envelope.

Evidence:

  • rust/src/application/commands/mod.rs:584-660

So godaddy application deploy --follow no longer guarantees final type: "result" / type: "error".


7. Built-in auth is not equivalent

The port relies on cli-engine built-in auth. That auth surface is shaped around:

auth login --env <ENV> [--provider <NAME>]

Evidence:

  • cli-engine/src/auth/commands.rs:39-75

The current GoDaddy CLI supported:

godaddy auth login
godaddy auth login --scope commerce.orders:read

The Rust auth provider hardcodes app-registry scopes and does not implement --scope.

Evidence:

  • Hardcoded scopes: rust/src/auth.rs:13
  • Provider construction ignores env var overrides mentioned in comments: rust/src/auth.rs:27-47

So scope-requesting workflows and API auto-reauth are missing.


API command regressions

8. godaddy api call is heavily regressed

Missing/broken behavior from the current CLI:

  • Legacy compatibility godaddy api /path is gone.
  • Endpoint catalog resolution is gone.
  • Method inference from catalog is gone.
  • Trusted absolute URL handling is gone.
  • Untrusted absolute URL rejection is gone.
  • -H / --header is parsed but never applied to the request.
  • --scope no longer triggers re-auth and retry; it only emits a hint after a 403.
  • Non-2xx responses are returned as successful command results instead of error envelopes.
  • Non-JSON responses are silently converted to null.
  • Response headers from --include are not sanitized, so sensitive headers may leak.
  • Output shape changed from { endpoint, method, status, status_text, data } to { status, body }.
  • GraphQL endpoint detection/error handling is gone.

Evidence:

  • URL is naïvely base_url + endpoint: rust/src/api_explorer/mod.rs:451-452
  • Header arg is declared but never used in request construction: rust/src/api_explorer/mod.rs:405-411, rust/src/api_explorer/mod.rs:492-500
  • Response body is forced through JSON only: rust/src/api_explorer/mod.rs:539
  • Only 403 with scopes errors; all other bad statuses are returned as success data: rust/src/api_explorer/mod.rs:541-562

9. api describe/list/search lost agent-safe output

Regressions:

  • No truncation or full_output.
  • No schema summarization.
  • No schema_detail file for complex schemas.
  • No GraphQL summary.
  • No --method/-m on api describe.
  • Fuzzy multiple-match behavior is gone.
  • api list --domain returns a bare array instead of { domain, endpoints, total, shown, truncated }.
  • api list returns a bare array instead of { domains, total, shown, truncated }.
  • api search returns a bare array and no totals/truncation.

Evidence:

  • Rust list returns raw arrays: rust/src/api_explorer/mod.rs:203-236
  • Rust describe returns raw schema: rust/src/api_explorer/mod.rs:274-285
  • Rust search returns raw array: rust/src/api_explorer/mod.rs:314-326

Application feature parity gaps

Again: the new command syntax is acceptable if intentional. These are issues where equivalent functionality is missing.

10. application release no longer releases configured actions/subscriptions

Current release reads godaddy.toml and sends configured actions and subscriptions in createRelease.

Rust release only sends:

{
  "applicationId": "...",
  "version": "..."
}

Evidence:

  • Rust release payload: rust/src/application/commands/mod.rs:531-544
  • Current release loads config actions/subscriptions: src/core/applications.ts:849-872

Impact: action hooks and webhook subscription configs added via application add are not included in releases.


11. application deploy does not activate the application

Current deploy ends by updating application status to ACTIVE. Rust deploy uploads extensions and stops. There is no update_application(... status ACTIVE ...).

Evidence:

  • Rust deploy completes after extension loop only: rust/src/application/commands/mod.rs:637-660
  • Current deploy activates app: src/core/applications.ts:1324-1336

Impact: deployment may upload artifacts but leave the app inactive.


12. No-extension deploy no longer does the useful thing

Current behavior: if no extensions are configured, deploy still activates the app and returns a deploy result.

Rust behavior: zero extensions -> emits completed -> no activation.

Evidence:

  • Rust no-extension path just completes: rust/src/application/commands/mod.rs:637-660
  • Current no-extension path activates app: src/core/applications.ts:1023-1045

13. Extension targets are missing/broken

Current application add extension embed/checkout requires --target and writes target entries. Rust removed --target entirely and writes targets: [].

Evidence:

  • Rust embed args omit target and write empty targets: rust/src/application/commands/mod.rs:924-965
  • Rust checkout args omit target and write empty targets: rust/src/application/commands/mod.rs:973-1014
  • Config type defaults targets to empty: rust/src/config/mod.rs:65-80

Deploy then uploads using ext_name as target, not the configured extension targets.

Evidence:

  • Upload target set to extension name: rust/src/application/commands/mod.rs:785-791
  • Current deploy uses configured targets, or blocks: src/core/applications.ts:1212-1220

Impact: embed/checkout extension artifacts are uploaded to wrong target locations, or target placement is lost entirely.


14. application init lost major behavior

Missing from Rust:

  • --config
  • --environment
  • reading defaults from existing config
  • .env.<env> creation
  • writing GODADDY_WEBHOOK_SECRET, GODADDY_PUBLIC_KEY, GODADDY_CLIENT_ID, GODADDY_CLIENT_SECRET
  • public URL validation equivalent to publicHttpUrl
  • preserving empty subscriptions.webhook

Evidence:

  • Rust required args/write-only-config path: rust/src/application/commands/mod.rs:118-213
  • Current config + env file behavior: src/core/applications.ts:510-543

15. application validate is not equivalent

Current application validate <name> validates the remote application state.

Rust application validate only deserializes local TOML.

Evidence:

  • Rust: rust/src/application/commands/mod.rs:233-260
  • Current: src/core/applications.ts:651-704

If the new design wants local config validation, that is fine, but the old remote validation feature is missing.


16. application update lost status updates

Current CLI supports:

godaddy application update <name> --status ACTIVE|INACTIVE

Rust update supports only --label and --description.

Evidence:

  • Rust args: rust/src/application/commands/mod.rs:264-287
  • Current update input includes status: src/core/applications.ts:706-760

Security scanning concerns

17. Security scanning port is not equivalent

Current deploy does:

  1. Pre-bundle package scan:
    • package.json scripts
    • source file discovery
    • AST-based alias-aware rules SEC001-SEC010
    • excludes node_modules, dist, build, __tests__
  2. Bundle
  3. Post-bundle scan SEC101-SEC110
  4. Upload

Rust deploy does:

  1. Bundle
  2. Regex scan bundled bytes only
  3. Upload

Missing from Rust:

  • Pre-bundle source scan.
  • Package script scanning (SEC011).
  • AST/alias-aware detection.
  • Security config and exclude patterns.
  • Source-level rules SEC001-SEC010.
  • Structured scan reports.
  • Separate pre-bundle and post-bundle reports.
  • Cleanup/reporting behavior around blocked bundles.

Evidence:

  • Current orchestration: src/services/extension/security-scan.ts:17-76
  • Current deploy prebundle scan: src/core/applications.ts:1052-1129
  • Current deploy postbundle scan: src/core/applications.ts:1163-1206
  • Rust only scans bundled bytes: rust/src/application/commands/mod.rs:734-766
  • Rust scanner comment says bundle rules only: rust/src/extension/mod.rs:40-43

18. Rust adds unreviewed blocking rules SEC111-SEC115

The Rust file says it ported SEC101-SEC110, but it also adds SEC111-SEC115 as blocking rules:

  • SEC111 destructive fs ops
  • SEC112 untrusted domains
  • SEC113 any base64/hex payload
  • SEC114 debugger
  • SEC115 dynamic require/import

Evidence:

  • Comment says SEC101-SEC110: rust/src/extension/mod.rs:40-43
  • Extra rules: rust/src/extension/mod.rs:250-310

Impact: this is not a straight port. It can create new false positives and block legitimate deployments without docs or compatibility tests.


19. Bundler port is incomplete

Missing/different from current behavior:

  • No artifact report with artifact path/name/size.
  • No persistent artifact path; bundle exists only in memory.
  • No upload-size validation.
  • No upload retry logic.
  • No cleanup semantics matching current implementation.
  • No use of handle as artifact name.
  • Only first .mjs output is used; code splitting/multiple outputs are not handled.
  • No visible local tsconfig handling.
  • No tests for existing extension fixtures.

Evidence:

  • Rust bundler returns only bytes + sha: rust/src/extension/mod.rs:19-22
  • Rust finds first .mjs: rust/src/extension/mod.rs:517-528
  • Current deploy has richer bundle/upload metadata and cleanup: src/core/applications.ts:1131-1308

20. S3 upload behavior is incomplete

Current upload:

  • validates maxSizeBytes
  • retries transient/network/5xx failures
  • strips x-amz-meta-upload-id
  • returns upload metadata
  • uses structured typed errors

Rust upload:

  • no max size check
  • no retries
  • no metadata result
  • sends all required headers as-is
  • no content type option
  • no special S3 header handling

Evidence:

  • Rust upload call: rust/src/application/commands/mod.rs:798-813
  • Current upload behavior: src/services/extension/upload.ts

Config/environment/output regressions

21. Config validation is gutted

Rust only Serde-deserializes TOML. It does not validate:

  • name pattern
  • UUID client_id
  • semver version
  • URL shape
  • non-empty scopes
  • action/subscription names min length
  • endpoint URL relative to proxy
  • extension targets non-empty for embed/checkout

Evidence:

  • Rust config structs/read: rust/src/config/mod.rs:3-131
  • Current ArkType schema validation: src/services/config.ts:1-128

Impact: invalid configs pass validation and then fail later or ship bad releases.


22. Environment behavior regressed

Regressions:

  • env info [environment] no longer accepts an environment arg.
  • env info no longer reports config file or config summary.
  • env get output changed from { environment } to { env, apiUrl }.
  • env list output changed shape.
  • GODADDY_API_BASE_URL override support is gone.
  • GODADDY_OAUTH_CLIENT_ID override support is gone.
  • global --env accepts any string and only fails later in auth provider; API URL silently falls back to OTE for unknown env.

Evidence:

  • Rust only registers global --env as a string: rust/src/main.rs:31-45
  • Rust api_url_for defaults unknown to OTE: rust/src/env/mod.rs:26-30
  • Rust env info has no args/config summary: rust/src/env/mod.rs:99-109

23. Webhook output lost truncation and normalized shape

Current CLI maps API response to events, truncates output, and returns total/shown/truncated/full_output.

Rust returns raw API JSON.

Evidence:

  • Rust raw return: rust/src/webhook/mod.rs:32-40
  • Current truncation: src/cli/commands/webhook.ts:37-51

24. Actions output lost context protection

Regressions:

  • actions list no longer returns total/shown/truncated/full_output.
  • actions describe returns raw schema without protectPayload.
  • Parent/group discovery output depends on cli-engine help instead of explicit JSON command tree.

Evidence:

  • Rust actions list/describe: rust/src/actions_catalog/mod.rs:69-101
  • Current protected/truncated output: src/cli/commands/actions.ts:69-128

25. HATEOAS/next_actions are incomplete or incompatible

Many next_actions are now incomplete or less useful:

  • Missing godaddy prefix in many actions.
  • Some use new flags (--name, --application-id, --version) without params metadata.
  • Some suggested actions point to commands that are no longer equivalent.
  • Error handling uses generic CliCoreError::message, so errors are not mapped to stable GoDaddy codes/fixes.

Evidence examples:

  • application info --name <name>: rust/src/application/commands/mod.rs:55
  • application release --application-id <id> --version <ver>: rust/src/application/commands/mod.rs:88
  • application validate next action with no name/config context: rust/src/application/commands/mod.rs:221
  • Generic error mapping helper: rust/src/application/commands/mod.rs:19-20

CI/release/testing concerns

26. Release/distribution changed incompatibly

The repo currently publishes @godaddy/cli to npm. This PR deletes package.json, the lockfile, build scripts, and changes release to GitHub binary artifacts on tags.

Unless this is intentional and separately migrated, this breaks:

npm install -g @godaddy/cli

Evidence:

  • package.json deleted in PR.
  • Release workflow changed from changesets/npm publish to tag binary release: .github/workflows/release.yaml

27. Code scanning/CI concerns

The CodeQL port is thin:

  • Downgrades CodeQL action version relative to base workflow.
  • Removes matrix/fail-fast structure.
  • Adds explicit Rust build, but no replacement dependency/security audit for Rust supply chain.
  • Since most tests are deleted, CodeQL/clippy/test are not enough to prove feature parity.

Evidence:

  • New CodeQL: .github/workflows/codeql-analysis.yml
  • CI only runs cargo check/fmt/clippy/test: .github/workflows/cicd.yml

28. Test coverage was effectively deleted

Current repo has extensive tests for:

  • auth
  • CLI smoke
  • API catalog
  • API command behavior
  • environment
  • config routing
  • deploy streaming
  • source security rules
  • bundle security rules
  • extension bundling
  • upload
  • GraphQL error mapping

PR deletes all TS tests and replaces them only with in-file Rust scanner tests.

Impact: most regressions above would not be caught.


Bottom line

This PR is not feature-equivalent yet. Even accepting the new application command syntax, it currently breaks or drops major behavior:

  1. API catalog refs unresolved.
  2. Root discovery is not JSON.
  3. Envelope contract changed.
  4. Error envelopes go to stderr.
  5. --output reintroduced / --pretty missing.
  6. Streaming lacks terminal result/error event.
  7. Auth scope workflows missing.
  8. API call functionality/security regressed.
  9. Release omits actions/subscriptions.
  10. Deploy does not activate apps.
  11. Extension targets missing.
  12. Security scanning is not equivalent.
  13. Config validation is not equivalent.
  14. npm distribution removed.
  15. Tests were mostly deleted.

I would not merge this until the Rust port either restores feature parity or the PR is explicitly reframed as a breaking new CLI with migration docs and compatibility expectations reset.

@jpage-godaddy jpage-godaddy marked this pull request as draft June 2, 2026 18:31
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.

4 participants