[icanhasmath]

Fixes for state publish / state export ingredient-meta found while testing 0.48.2-RC1 (PROD-240). Targets version/0-48-2-RC2, which is beta (0.48.2-RC1) plus the version.txt bump.

Export output was not a valid meta file. state export ingredient-meta printed the publish prompt's yaml, with a # ... help comment above every field, through the plain outputer, which word-wraps to the terminal width in an interactive shell. The wrapped comments turned into non-comment lines. It now writes plain yaml straight to stdout.

Expecting an OBJECT with a field named 'versionRequirements'. A dependency without a version range (a bare requests in pyproject.toml, --depend without a version, or a condition such as kernel/Windows inherited without requirements, which also made the exported file incomplete) had an empty versionRequirements, which is omitted from the request. Empty requirements are now >=0, both when publishing and when exporting.

Unclear error when pyproject.toml lacks the name or version. The error now lists each missing field, why it wasn't found (no pyproject.toml, not in [project], or listed in [project].dynamic), and where to set it (pyproject.toml, --name/--version, or a --meta file).

Fixing the meta data before publishing. The Y/n confirmation is unchanged (defaults, -n and --force behave as before), but answering n now offers:

• Edit it in my editor: opens it in $EDITOR, then asks again. With --build or --edit, changing the name, namespace or version there would no longer match the built package or the looked-up ingredient, so those edits are saved to a file with instructions to re-run instead.
• Save it to a meta file: writes <name>-<version>.meta.yaml (asks before replacing) and prints the command to publish it, e.g. state publish --build ./src --meta mypkg-1.0.0.meta.yaml.
• Cancel (default).

Every pass of the confirmation re-adds the private-builder for --build, fills empty requirements, and checks the namespace and that a revision's authors and description are unchanged. An edit can't skip those checks.

Also: fields deleted in the editor are now removed from the publish instead of keeping their old values, and a failed publish shows the API error instead of Error received: <no value> (en-us.yaml's err_uploadingredient_publish needs it as {{.V0}}).

Security: github.com/labstack/echo/v4 4.15.4 → 4.16.1 (GHSA-cxf3-9h37-7rr8 critical, GHSA-9969-6676-49h5, GHSA-f48p-c9v4-h7xv, GHSA-h7wr-284f-5qvc), which failed the Scan job.

Not changed: --edit still needs the source. The publish mutation requires file on every publish and has no way to reference the existing upload, so a metadata-only edit would need a platform change. The readme now says to pass --build with --edit.

Testing

• Each fix has a test through its call path that fails if the fix is reverted: confirmRequest with a fake prompter and the test binary as $EDITOR (filling requirements, removing fields, cleared namespace, description on a revision, private-builder, save, cancel), publishFailedError, metaFromIngredient and writeIngredientMeta.
• Unit tests for the missing-metadata error and the re-run command.
• The Cancel Publish integration test selects the default Cancel after answering n.
• Also in this PR: echo v4.16.1 (Scan), and a fix for internal/hail TestOpen failing intermittently on Windows (it removed the file while the watcher could still be reading it).
• Not yet run end to end against the platform.

🤖 Generated with Claude Code

[icanhasmath]

Restored the errs.JoinMessage(err) argument to err_uploadingredient_publish in d1d6fa3. The en-us.yaml entry ends in Error received: {{.V0}} and overrides the fallback text, so without the argument the user sees Error received: <no value> (as in the RC1 test). The wrapped API error isn't user-facing, so it only reaches the log. There's now a code comment explaining this. The echo v4.16.1 bump in 2663339 is unchanged.

[icanhasmath]

Agreed on the fix in d1d6fa3, and I got this wrong in 2663339. err_uploadingredient_publish (en-us.yaml:1413-1417) does end in Error received: {{.V0}}. My read of the file stopped one line short of it, so removing the argument would have brought back <no value>.

[importance 3 / effort 1] The new code comment's reasoning doesn't match the code for the main failure path. bp.Publish returns the client.Run failure through processBuildPlannerError (pkg/platform/model/buildplanner/build.go:158). That gives back either the input error unchanged or a *response.BuildPlannerError, and both are user-facing: BuildPlannerError implements LocaleError() (pkg/platform/api/buildplanner/response/shared.go:29). OutputError.MarshalOutput (internal/runbits/errors/errors.go:67-80) prints every ErrorLocalizer that locale.UnpackError finds in the chain. So an API failure shows its text twice: once after Error received: (as the YAML that JoinMessage produces) and again as its own line. Only a failure inside request.Publish (an errs.Wrap, which isn't user-facing) is shown solely through {{.V0}}.

Keeping the argument is still right, because the template needs it. Suggested wording:

// en-us.yaml's err_uploadingredient_publish ends in "Error received: {{.V0}}" and overrides the fallback text, so the
// message must be passed in, or the user sees "<no value>".

Or, to stop the duplicate on the API path, pass errs.JoinMessage(err) only when !locale.HasError(err) and "" otherwise. That would also need a matching tweak to the template, so it's optional.

[importance 1 / effort 1] The PR body no longer mentions the <no value> fix. I removed it there along with the changelog line. The changelog has it again, so the body should too.

[icanhasmath]

[importance 3 / effort 1] The second half of this comment doesn't hold for an API failure. bp.Publish returns client.Run errors through processBuildPlannerError (pkg/platform/model/buildplanner/build.go:158). That gives back either the input error unchanged or a *response.BuildPlannerError, and both are user-facing: BuildPlannerError implements LocaleError() (pkg/platform/api/buildplanner/response/shared.go:29). OutputError.MarshalOutput (internal/runbits/errors/errors.go:67-80) prints every ErrorLocalizer that locale.UnpackError finds in the chain, so the API error is printed beneath this message as well as inside {{.V0}}. Only a request.Publish failure (an errs.Wrap) is shown solely through {{.V0}}.

The argument is still needed. Suggested wording:

Suggested change

	// en-us.yaml's err_uploadingredient_publish ends in "Error received: {{.V0}}" and overrides the fallback text, so
	// the message must be passed in: without it the user sees "<no value>", and the wrapped API error is not
	// user-facing, so it would otherwise only appear in the log.
	// en-us.yaml's err_uploadingredient_publish ends in "Error received: {{.V0}}" and overrides the fallback text, so
	// the message must be passed in, or the user sees "<no value>".

[icanhasmath]

Agreed for client.Run failures: processBuildPlannerError returns either the input error or a *response.BuildPlannerError, both user-facing, so those print beneath the message as well.

The RC1 failure took the other path, though. "File to upload was not provided" came back in the publish result, and bp.Publish turns that into errs.New("API responded with error: %s", res.Error) (pkg/platform/model/buildplanner/publish.go:21; the RC1 trace's "Created at" points there). That error isn't user-facing, so {{.V0}} is the only place it reaches the user.

I left out the suggested wording because it drops that case. bdd6c7d rewrites the comment to say the argument prevents <no value>, and that it's the only place an error from the publish result gets shown, unlike a BuildPlannerError.

[MDrakos]

Requesting changes because edits made in the new confirm loop skip the checks that run before it.

The fixes this PR is about aren't guarded by tests. With the fillVersionRequirements calls in Run and FetchMeta removed, OpenInEditor back to the in-place unmarshal, and the errs.JoinMessage(err) argument dropped, go test ./internal/runners/publish/ still passes. TestFillVersionRequirements only covers the helper, and nothing runs export ingredient-meta, so the plain-yaml export could be reverted with CI staying green. I think one test per fix through the real call path (a fake prompt.Prompter works for the loop, and $EDITOR pointed at a script for the editor path) would keep these from regressing before the next RC.

1 inline comment below.

[MDrakos]

An edit made in the new confirm loop skips the checks that ran before it. If you clear namespace in the editor you get the vague "either Path of Name and Namespace are required" from MarshalJSON, and checkEditKeepsIdentity never sees authors or description added on a revision. imo the simplest fix is to run both checks at the top of each pass:

Suggested change

	// Until the user confirms, they can edit the prepared meta data or save it to a file and start over with it.
	for {
	if params.Build != "" {
	addPrivateBuilderDependency(&reqVars) // again, in case it was removed in the editor
	}
	fillVersionRequirements(&reqVars)
	// Until the user confirms, they can edit the prepared meta data or save it to a file and start over with it.
	for {
	if params.Build != "" {
	addPrivateBuilderDependency(&reqVars) // again, in case it was removed in the editor
	}
	fillVersionRequirements(&reqVars)
	if reqVars.Namespace == "" {
	return locale.NewInputError("err_uploadingredient_namespace_required", "You have to supply the namespace when working outside of a project context")
	}
	if err := checkEditKeepsIdentity(&reqVars, params.Edit, isRevision); err != nil {
	return err
	}

[icanhasmath]

Agreed, done in 8ecef8d. The loop is now confirmRequest, and each pass re-adds the private-builder (for --build), fills empty version requirements, then checks the namespace and checkEditKeepsIdentity before showing the request. I removed the pre-loop copies of both checks, since the first pass now does them. One small change in behaviour: with --editor, a description given on a revision is now rejected after the editor closes instead of before it opens.

New tests cover both cases through the editor: a cleared namespace gives the namespace error, and a description added on a revision gives the identity error.

[icanhasmath]

@MDrakos thanks. I've added tests through the real call paths in 8ecef8d. Each one fails when its fix is reverted (I checked each by hand):

Reverted	Failing test
fillVersionRequirements in the confirm loop	TestConfirmRequestFillsVersionRequirements, TestConfirmRequestSave
fillVersionRequirements in FetchMeta	TestMetaFromIngredient
OpenInEditor back to unmarshalling in place	TestConfirmRequestEditor/removed_fields_are_removed
errs.JoinMessage(err) argument dropped	TestPublishFailedError
identity check removed from the loop	TestConfirmRequestEditor/description_added_on_a_revision_is_rejected
export back to MarshalYaml	TestWriteIngredientMeta (in internal/runners/export)

The loop tests run confirmRequest with a fake prompt.Prompter. For the editor, $EDITOR points at the test binary itself: a TestMain hook writes the given content to the file, so the tests also run on Windows. To test without the API, I moved the part of FetchMeta that builds the meta data into metaFromIngredient, and the export's writing into writeIngredientMeta(io.Writer, …). That last test checks the YAML (no comments, a long description with [TAGS] round-trips). It doesn't cover Run writing to OutWriter instead of calling Print, since Run needs the API.

On the earlier note about duplicates: an error from a failed request now shows twice (inside Error received: and on its own line), while an error from the publish result shows once. I left that as it is. Removing the duplicate would mean changing the template for one path, which is beyond this PR. The PR body mentions the <no value> fix again.