Skip to content

fix: align leftovers with the command conventions - #1653

Merged
Lena Forlin (moshimorschi) merged 2 commits into
mainfrom
ux/convention-leftovers
Oct 1, 2026
Merged

Lena Forlin (moshimorschi) merged 2 commits into
mainfrom
ux/convention-leftovers

Conversation

@moshimorschi

@moshimorschi Lena Forlin (moshimorschi) commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

What changed?

Leftovers against docs/COMMAND_CONVENTIONS.md, no new behaviour except for stray arguments:

  • Enumerated values in help are written (a, b, c) without "or": project create --deployment and --ci, project sql --format, and the (table, json) lists on project extension list, project extension outdated and account producer extension list.
  • Errors no longer carry Go identifiers: PrepareFolderForZipping, prepareShopwareForAsset (now names the file it could not create or write), downloadFileTo and the uppercase Unzip:.
  • Four errors wrap their cause with %w instead of %v and read cannot ...: public key generation, the changelog template, and the two store-push file errors.
  • Three log lines are capitalised sentences: the ldd warning (also fixes the requierd typo), the local override warning and the deprecated-login warning.
  • project worker declares at most one argument, project upgrade-check none, project admin-api explicitly any, since --output-token runs without arguments and extra ones are passed to curl.

Screenshots

zip-message worker-args upgrade-check-args help-enums

Why?

These are the spots the conventions guide still disagreed with after the September passes. Flags, defaults and output formats are untouched; the only visible change for users is that worker and upgrade-check now reject a stray argument instead of silently ignoring it.

How was this tested?

  • go test ./..., golangci-lint run ./... and gofmt clean; one existing assertion updated to the new wording.
  • Built binary: the changed help lines, project worker 5 extra and project upgrade-check stray fail with exit 1, project admin-api --output-token and project admin-api GET /path <curl args> behave as before.
  • Dry-run merge against chore (cli): clarify command and flag help texts #1646 is clean; the project validate --format wording and the shared flag sentences are left to that PR.

Related issue or discussion

#1652, #1642

Summary by CodeRabbit

  • Command-Line Improvements

    • Updated format and option help text to present supported choices more clearly.
    • The upgrade-check command now rejects positional arguments, and the worker command rejects more than one.
  • Error Messages and Diagnostics

    • Improved context in errors for key generation, downloads, file processing, changelog generation, extension packaging, and archive path violations.
    • Clarified a warning about legacy authentication and corrected wording in a download-related warning.

- enumerated values in help are listed with commas, without "or"
- errors no longer carry Go identifiers or an uppercase prefix
- four errors wrap their cause with %w and read "cannot ..."
- three log lines are capitalised sentences, one typo fixed
- project worker, upgrade-check and admin-api declare their argument arity
@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 6 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: eff42308-1eaa-46e5-af54-0828f9054a8c

📥 Commits

Reviewing files that changed from the base of the PR and between 8c622a5 and 2b9b94b.

📒 Files selected for processing (1)
  • internal/account-api/login.go

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 22edeee6-51f1-4ffb-b6e6-ce710c50d8c3

📥 Commits

Reviewing files that changed from the base of the PR and between b1375e4 and 8c622a5.

📒 Files selected for processing (19)
  • cmd/account/account_producer_extension_list.go
  • cmd/project/project_admin_api.go
  • cmd/project/project_create.go
  • cmd/project/project_extension_list.go
  • cmd/project/project_extension_outdated.go
  • cmd/project/project_generate_jwt.go
  • cmd/project/project_sql.go
  • cmd/project/project_upgrade_check.go
  • cmd/project/project_worker.go
  • internal/account-api/login.go
  • internal/account-api/producer_store_pull.go
  • internal/account-api/producer_store_push.go
  • internal/account-api/producer_store_push_test.go
  • internal/archiver/zip.go
  • internal/changelog/changelog.go
  • internal/esbuild/download_unix.go
  • internal/extension/asset_platform.go
  • internal/extension/zip.go
  • internal/shop/config.go

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The changes update CLI argument validation and help text, error wrapping and context, and log and warning messages. Most updates change messages; several errors now preserve the underlying error for unwrapping.

Changes

CLI and error reporting

Layer / File(s) Summary
CLI validation and help text
cmd/account/*, cmd/project/*
CLI commands set or update positional-argument validation. Flag descriptions also receive wording and format-list updates.
Error wrapping and operation context
cmd/project/project_generate_jwt.go, internal/account-api/producer_store_push.go, internal/account-api/producer_store_push_test.go, internal/archiver/zip.go, internal/changelog/changelog.go, internal/extension/asset_platform.go, internal/extension/zip.go
Several errors now wrap the underlying error with %w. Error prefixes and operation or file context also change. The missing-file test checks for the updated error wording.
Log and warning messages
internal/account-api/login.go, internal/account-api/producer_store_pull.go, internal/esbuild/download_unix.go, internal/shop/config.go
Authentication, download, esbuild, and local-config log or warning messages receive wording updates. The esbuild message changes from info level to warning level.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: shyim

Merge Risk: ⚪ Minimal · up to 8c622

The command argument rules match the described interfaces, and the remaining changes clarify help text and errors without an identified behavior regression. No merge-blocking risk is evident.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 8c622

The reviewed changes tighten argument validation and improve error reporting without expanding credential access or file-writing authority. No material security risk was found to be introduced or worsened.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The inspected changes do not widen authenticated request authority or archive destination authority. Invocation restrictions narrow accepted worker and upgrade-check inputs; archive path validation and extraction operations retain their previous exposure.

Trust Boundaries and Controls

  • observed — NewApi retains cached-token, client-credential, legacy-credential, and interaction-gated login selection; its PR change is warning text. Admin-api retains its credential check, token acquisition, caller-supplied curl arguments, and configured TLS-check behavior. These authority-bearing paths are not introduced by this PR.

Resilience and Maintainability Implications

  • observed — Archive extraction remains sequential and non-transactional: earlier extracted files can remain after a later failure, with no new cancellation or synchronization mechanism. These failure-containment limitations are unchanged from the reviewed commit's parent.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 23.53% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 19 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title clearly summarizes the main change: aligning remaining command behavior and wording with project conventions.
Description check ✅ Passed The description includes all required sections, explains the changes and rationale, documents testing, and references related issues. It also provides screenshots for CLI output changes.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@codecov-commenter

Codecov Comments Bot (codecov-commenter) commented Oct 1, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 45.00000% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.73%. Comparing base (5731e29) to head (2b9b94b).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
internal/extension/asset_platform.go 0.00% 4 Missing ⚠️
cmd/project/project_generate_jwt.go 0.00% 1 Missing ⚠️
internal/account-api/producer_store_pull.go 0.00% 1 Missing ⚠️
internal/account-api/producer_store_push.go 50.00% 1 Missing ⚠️
internal/changelog/changelog.go 0.00% 1 Missing ⚠️
internal/esbuild/download_unix.go 0.00% 1 Missing ⚠️
internal/extension/zip.go 0.00% 1 Missing ⚠️
internal/shop/config.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1653      +/-   ##
==========================================
- Coverage   65.74%   65.73%   -0.02%     
==========================================
  Files         462      462              
  Lines       30949    30949              
==========================================
- Hits        20347    20343       -4     
- Misses      10602    10606       +4     
Flag Coverage Δ
go-test 65.73% <45.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

just two edits; thank you Lena Forlin (@moshimorschi)!

Comment thread cmd/project/project_create.go
Comment thread internal/account-api/login.go Outdated
@moshimorschi
Lena Forlin (moshimorschi) merged commit fae9eec into main Oct 1, 2026
5 checks passed
@moshimorschi
Lena Forlin (moshimorschi) deleted the ux/convention-leftovers branch October 1, 2026 11:47
@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

Warning

Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.

What happened

The threat detection engine failed to produce results.

Review the workflow run logs for details.

Reviewed this PR against shopware/docs (concepts/, guides/, products/).

Changed files: help text wording ((a, b, c) instead of (a, b, or c)), error message wording/wrapping (%w + lowercase cannot ...), log line capitalization, and cobra.Args validators added to project worker, project upgrade-check, and project admin-api.

Why no doc update: These are text-only polish and internal error-wrapping changes per docs/COMMAND_CONVENTIONS.md, with no new flags, defaults, or output formats. The only behavioral change — worker now rejecting more than one argument and upgrade-check rejecting any argument — only enforces the single-argument usage already shown in products/tools/cli/project-commands/helper-commands.md (shopware-cli project worker <amount>) and the no-argument usage shown for upgrade-check in guides/hosting/installation-updates/{performing-updates,docker}.md. No existing docs describe or rely on passing extra/stray arguments, so current documentation remains accurate and no update is required.

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