Skip to content

fix(cli): classify usage errors in top failure-parameter blind spots - #2119

Open
rudrankriyam wants to merge 6 commits into
mainfrom
fix/classify-top-usage-errors
Open

fix(cli): classify usage errors in top failure-parameter blind spots#2119
rudrankriyam wants to merge 6 commits into
mainfrom
fix/classify-top-usage-errors

Conversation

@rudrankriyam

@rudrankriyam rudrankriyam commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator

Summary

Telemetry shows these commands emit unclassified usage errors at the highest rates: they name the offending flag on stderr, but return a bare fmt.Errorf or flag.ErrHelp, so failure_parameter and diagnostic_code arrive empty and aggregate data cannot see which flag trips callers.

This sweeps the structured-diagnostics convention (shared.WithDiagnostic / classified usage errors, established in #2032-#2043) into the top offenders. Every change is additive metadata only.

No user-visible change. Rendered stderr text, flag.ErrHelp usage semantics, and exit codes are identical. Each command's current stderr and exit contract is pinned in a test before the classification assertion, so a message or exit-code drift fails the build.

Per-command status

Command Change
asc web privacy pull Missing --app now carries required_input_missing / --app
asc versions links Out-of-range --limit, non-ASC --next, unknown --type, and pagination flags on to-one relationships now carry invalid_input / conflicting_input with the exact flag
asc pricing schedule create Empty and conflicting price selection, non-numeric --price, unmappable --base-territory, malformed --start-date
asc web apps create Missing --name / --bundle-id / --sku in a non-interactive terminal, and missing --apple-id with no cached session
asc web review show Missing --app, malformed --pattern, unknown --submission
asc review submit Already fully classified at HEAD — no production change. Added a characterization test so a future edit cannot silently drop a failure parameter

Notes on approach

  • ValidatePriceSelectionFlags now attaches the diagnostic for the rule it actually enforced (negative --tier vs. nothing-selected vs. mutually-exclusive), and the shared pricing set builder forwards it onto flag.ErrHelp instead of re-deriving which rule fired. Its other callers discard the returned error's structure, so their behavior and telemetry are unchanged.
  • Multi-parameter requirements stay deliberately unattributed (explicit empty string) per the existing contract convention. web apps create names the flag only when exactly one is missing rather than guessing among several.
  • Four new entries in the failure_parameter allowlist — apple-id, pattern, sku, tier — each registered in internal/telemetry/event.go and covered by the existing TestBuildEventWithContextAllowsKnownFailureParameters list, per that contract's own convention.

Covered elsewhere

asc xcode-cloud * and asc web review subscriptions list are also high-volume unclassified emitters but are owned by other in-flight work, so they are deliberately untouched here.

Testing

  • make build, make format, make check-docs, make lint (0 issues), ASC_BYPASS_KEYCHAIN=1 make test — all green, no pre-existing failures observed.
  • Smoke-tested the built binary for each touched command: stderr text and exit codes (2 for usage, 1 for the plain --limit failure) match pre-change behavior exactly.
  • No live API calls; every case fails at local validation before any network work.

Summary by CodeRabbit

  • Improvements
    • CLI validation errors now provide structured diagnostic details, including the relevant input and flag.
    • Improved guidance is available for missing, conflicting, malformed, or unsupported inputs across pricing, review, version, and web commands.
    • Help and usage errors now preserve diagnostic information for clearer troubleshooting.
    • Additional failure parameters are recognized in telemetry, improving error reporting consistency.
  • Testing
    • Expanded coverage verifies diagnostic output, error classifications, usage behavior, and stderr messages across affected commands.

asc web privacy pull rejected a missing --app with an unclassified usage
error, so telemetry recorded no failure parameter or diagnostic code for
one of the CLI's most frequent usage failures.

Attach the structured required-input diagnostic naming --app while
preserving the rendered stderr message, the flag.ErrHelp usage contract,
and the usage exit code.
asc versions links rejected out-of-range --limit, non-App-Store-Connect
--next, unknown --type, and pagination flags on to-one relationships
without any structured classification, so telemetry could not attribute
those failures to a flag.

Attach invalid-input and conflicting-input diagnostics naming the exact
flag while preserving each rendered message, the flag.ErrHelp usage
contract where it already applied, and every exit code.
asc pricing schedule create rejected an empty or conflicting price
selection, a non-numeric --price, an unmappable --base-territory, and a
malformed --start-date without any structured classification, so those
failures reached telemetry with no parameter or diagnostic code.

ValidatePriceSelectionFlags now carries the diagnostic for the rule it
enforced, and the shared pricing set builder forwards it onto the
flag.ErrHelp usage contract rather than re-deriving the failing rule.
Register --tier in the failure-parameter allowlist. Rendered messages,
usage semantics, and exit codes are unchanged.
asc web apps create rejected missing --name, --bundle-id, or --sku in a
non-interactive terminal, and a missing --apple-id with no cached web
session, without any structured classification, so telemetry recorded no
failure parameter for either.

Attach required-input diagnostics, naming the flag when exactly one is
absent and leaving multi-flag requirements unattributed rather than
guessing. Register --apple-id and --sku in the failure-parameter
allowlist. Rendered messages, usage semantics, and exit codes are
unchanged.
asc web review show rejected a missing --app, a malformed --pattern glob,
and an unknown --submission without any structured classification, so
telemetry could not attribute those failures to a flag.

Attach required-input, invalid-input, and resource-not-found diagnostics
naming the exact flag. Register --pattern in the failure-parameter
allowlist. Rendered messages, usage semantics, and exit codes are
unchanged.
asc review submit already classifies every one of its input failures, so
it needs no production change. Pin that contract with a characterization
test covering missing --app, --build, version selector, --confirm, the
mutually exclusive version selectors, and an unsupported --platform, so a
future edit cannot silently drop a failure parameter.

Verified the test has teeth by temporarily dropping the --confirm
parameter and watching the case fail.
@mintlify

mintlify Bot commented Aug 19, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
rudrankriyam-app-store-connect-cli-67 🟡 Building Aug 19, 2026, 12:39 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@coderabbitai

coderabbitai Bot commented Aug 19, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c1b26369-447c-489d-8d95-8e2ed8236988

📥 Commits

Reviewing files that changed from the base of the PR and between bbfeff8 and a3c6f22.

📒 Files selected for processing (13)
  • internal/cli/pricing/diagnostics_test.go
  • internal/cli/reviews/diagnostics_test.go
  • internal/cli/shared/errors.go
  • internal/cli/shared/pricing_command.go
  • internal/cli/shared/tier_resolver.go
  • internal/cli/versions/diagnostics_test.go
  • internal/cli/versions/relationships.go
  • internal/cli/web/diagnostics_test.go
  • internal/cli/web/web_apps_create_shared.go
  • internal/cli/web/web_privacy.go
  • internal/cli/web/web_review.go
  • internal/telemetry/event.go
  • internal/telemetry/event_test.go

Limit details: You’ve used the included review currently available. Your 71 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.


📝 Walkthrough

Walkthrough

The change adds structured diagnostic metadata to CLI validation errors across pricing, reviews, versions, and web commands. It preserves usage behavior and expands tests for error text, stderr output, diagnostic codes, parameters, and telemetry allowlist coverage.

Changes

Shared validation diagnostics

Layer / File(s) Summary
Shared validation diagnostics
internal/cli/shared/errors.go, internal/cli/shared/pricing_command.go, internal/cli/shared/tier_resolver.go, internal/cli/pricing/diagnostics_test.go, internal/cli/reviews/diagnostics_test.go
Validation errors now preserve structured diagnostics when converted to flag.ErrHelp. Pricing and review tests cover required, invalid, conflicting, and malformed inputs.

Versions relationship validation

Layer / File(s) Summary
Versions relationship validation
internal/cli/versions/relationships.go, internal/cli/versions/diagnostics_test.go
Relationship type, pagination, limit, and URL validation now reports diagnostic metadata. Tests verify errors, stderr, usage behavior, and parameters.

Web command diagnostics

Layer / File(s) Summary
Web command diagnostics
internal/cli/web/web_apps_create_shared.go, internal/cli/web/web_privacy.go, internal/cli/web/web_review.go, internal/cli/web/diagnostics_test.go
Web commands now annotate missing inputs, invalid patterns, and unknown submissions. Tests verify output, usage classification, diagnostic codes, and parameters.

Telemetry parameter allowlist

Layer / File(s) Summary
Telemetry parameter allowlist
internal/telemetry/event.go, internal/telemetry/event_test.go
The known failure-parameter allowlist now includes parameters emitted by the updated validation paths.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk: ⚪ Minimal · up to a3c6f

This PR adds diagnostic classification to existing CLI usage errors while preserving messages and exit codes; no actionable merge-blocking risk remains after normal checks and review.

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 37.04% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: classifying previously unclassified CLI usage errors and failure parameters.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/classify-top-usage-errors

Usage-based review receipt

Note

This review was completed with usage-based billing: files reviewed beyond your plan's included limits are billed at $0.25/file. Track spend and usage in your billing settings.


Comment @coderabbitai help to get the list of available commands.

@rudrankriyam rudrankriyam added the p2 Medium priority: useful fix with clear workaround or limited blast radius label Aug 19, 2026
@rudrankriyam rudrankriyam added this to the 4.6.1 milestone Aug 19, 2026
@rudrankriyam rudrankriyam added the medium Moderate scope with some cross-file or design work label Aug 19, 2026
@rudrankriyam rudrankriyam modified the milestones: 4.6.1, 4.8.2 Aug 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

medium Moderate scope with some cross-file or design work p2 Medium priority: useful fix with clear workaround or limited blast radius

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant