Skip to content

fix(cli): detect file upload params by type, and run CLI build on PRs#1209

Open
jablan wants to merge 3 commits into
mainfrom
fix/cli-binary-param-isfile
Open

fix(cli): detect file upload params by type, and run CLI build on PRs#1209
jablan wants to merge 3 commits into
mainfrom
fix/cli-binary-param-isfile

Conversation

@jablan

@jablan jablan commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

What

Two related fixes prompted by the screenshot/create endpoint breaking the CLI build (go build in clients/cli failed with params.Get*os.File and a too many arguments call).

1. Detect binary upload params by type, not by name

The CLI handlebars template detected file-upload params by matching the literal param name (file/filename). A required binary field with any other name fell through to the primitive handler and generated invalid Go:

filename := params.Get * os.File(helpers.ToSnakeCase("Filename"))

screenshot/create's binary field is named filename and is required, hitting exactly this case.

Both branches now use {{#isFile}} — the flag the generator derives from format: binary, and the same one the go/python/typescript/php templates in this repo already use. Future binary fields (image, attachment, …) work automatically.

2. Make CI actually compile the CLI on PRs

This break reached main because no CI job compiled the CLI on the PR:

  • test-cli.yml is the only job that runs go build on the CLI, but its paths: filter excluded the spec (paths/**, schemas/**, shared *.yaml) and the CLI templates the CLI is generated from — so a spec-only change skipped it. Switched to on: pull_request and expanded the path filter. (pull_request also covers fork PRs and associates the run with the PR as a gate.)
  • build.yml's build-cli job published the generated CLI to phrase-cli without ever compiling it. Added a go build (against the freshly generated go client, so it doesn't depend on a possibly-lagging released phrase-go) before the publish step.

Verification

  • Regenerated the CLI: screenshot/create (required filename) and upload/create (required file) both emit correct os.Open(...); no malformed output anywhere.
  • Built clients/cli against the freshly generated clients/go — clean build.

Note

The follow-on too many arguments error is a separate matter: the CLI's committed go.mod pins a released phrase-go that predates this endpoint. It resolves once a new phrase-go is released from the updated spec and the CLI bumps its dependency — unaffected by this PR.

🤖 Generated with Claude Code

The CLI handlebars template detected binary upload params by matching the
literal param name (file/filename). A required binary field named anything
else fell through to the primitive handler and emitted invalid Go
(params.Get*os.File), breaking the CLI build. The screenshot/create
endpoint, whose binary field is named "filename" and is required, hit
exactly this case.

Switch both the required and optional branches to {{#isFile}}, the flag the
generator derives from `format: binary` and that the go/python/typescript/php
templates here already use. File uploads are now detected by type, not name.

This break reached main because no CI job compiled the CLI on the PR:
- test-cli.yml (the only job that runs `go build` on the CLI) filtered on
  paths that exclude the spec and CLI templates the CLI is generated from,
  so a spec-only change skipped it. Switch it to pull_request and expand the
  path filter to cover paths/, schemas/, the shared *.yaml, and the CLI
  templates.
- build.yml's build-cli job published the generated CLI to phrase-cli without
  ever compiling it. Add a `go build` (against the freshly generated go
  client) before publishing so non-compiling code can't ship.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

API changelog (oasdiff)

Doc-only edits (descriptions, examples) do not appear here.

No changes detected

Copilot AI left a comment

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.

Pull request overview

This PR updates the OpenAPI-based CLI generation and CI workflows to (1) correctly treat binary/form-upload params as files based on OpenAPI type metadata, and (2) ensure the generated CLI is compiled in CI so spec/template changes can’t silently break builds.

Changes:

  • Update the CLI Handlebars template to detect file upload parameters via isFile instead of matching specific param names.
  • Update test-cli.yml to run on pull_request and expand the paths filter so spec/template changes trigger CLI compilation.
  • Update build.yml to generate the Go client and compile the generated CLI (against the freshly generated Go client) before publishing.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
openapi-generator/templates/cli/api.handlebars Switch file-upload handling to isFile to avoid invalid Go for required binary params.
.github/workflows/test-cli.yml Run CLI build/tests on PRs and broaden path filtering to catch spec/template-driven breaks.
.github/workflows/build.yml Generate Go client and compile generated CLI before publishing to prevent shipping broken artifacts.
Comments suppressed due to low confidence (1)

openapi-generator/templates/cli/api.handlebars:80

  • In the optional file-param branch, the code assigns localVarOptionals before checking err, which can store a nil/invalid file handle on failure. Also, for consistency with how flags are registered, prefer using vendorExtensions.x-export-param-name for the viper key.
						if params.IsSet(helpers.ToSnakeCase("{{paramName}}")) {
							file, err := os.Open(params.GetString(helpers.ToSnakeCase("{{paramName}}")))
							localVarOptionals.{{{vendorExtensions.x-export-param-name}}} = optional.NewInterface(file)
							if err != nil {
								HandleError(err)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread openapi-generator/templates/cli/api.handlebars Outdated
Comment thread .github/workflows/test-cli.yml
The file-upload branches read the value via ToSnakeCase(paramName) while
flags are registered via ToSnakeCase(x-export-param-name) (the AddFlag call).
These are identical for names like file/filename, but diverge when the
generator escapes a reserved word or disambiguates a name, in which case
viper never sees the flag value for the upload. Use x-export-param-name for
the lookup, matching registration and every other primitive param.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@jablan

jablan commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator Author

Good catch — fixed in a632435. Both file branches (required and optional) now read with helpers.ToSnakeCase("{{vendorExtensions.x-export-param-name}}"), the same key AddFlag registers with, instead of paramName. The Go local variable name stays paramName; only the viper lookup key changed.

This was a pre-existing inconsistency in the optional file branch that I copied into the new required branch — it happened to work because paramName == x-export-param-name for file/filename, but would have silently dropped the upload for any escaped/disambiguated name. Regenerated and verified screenshot/create and upload/create still build clean.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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.

2 participants