fix(plugin/external): pre-coerce string scalars before STRICT_PROTO protojson decode#652
Merged
Merged
Conversation
…rotojson decode
Workflow template expansion via {{ config "..." }} always emits strings,
even when the underlying config value is a scalar (bool / int / float).
Engine v0.51.x routes strict-proto step + module configs through
protojson.Unmarshal, which is strict: it rejects a string value for a
bool/int/float proto field with the canonical "invalid value for <type>
field X: \"<string>\"" error.
Before this fix, BMW (and any consumer with template-driven scalar
fields) hit:
Setup error: failed to build engine: ... STRICT_PROTO contract for
config message "workflow.plugins.auth.v1.AuthMethodsPolicyConfig"
cannot use legacy Struct fallback: decode AuthMethodsPolicyConfig
input as protobuf JSON: proto: invalid value for bool field
passwordAuthEnabled: "false"
Adds `coerceMapScalars` that walks the input map + proto field
descriptor and parses string values into the matching Go scalar
type before json.Marshal is called. Coverage:
- bool field: "true"/"false"/"1"/"0"/"yes"/"no"/"on"/"off" (CI)
- int* / sint*: strconv.ParseInt(value, 10, 64)
- uint* / fixed: strconv.ParseUint(value, 10, 64)
- float* / dbl: strconv.ParseFloat(value, 64)
- empty string for scalar: dropped (proto3 default)
- unparseable string: passthrough so protojson surfaces canonical
"invalid value for <type>" error on user typos
- non-string + unknown field: passthrough
- list/map fields: passthrough (nested coercion deferred)
Copy-on-coerce: the caller's map is not mutated.
Tests cover every kind, JSON-name vs proto-name keying, empty-string
drop, invalid passthrough, nil/empty input guards, and the BMW-shaped
regression (string "false" for bool default-valued field).
Closes the BMW PR 278 image-launch blocker on
\`step.auth_methods_policy\` + \`auth.credential\` configs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the external plugin STRICT_PROTO config decoding path by coercing template-emitted string scalars (e.g., "false") into the protobuf field’s expected scalar type before protojson.Unmarshal runs, preventing strict type-rejection during decode.
Changes:
- Added a
coerceMapScalarspre-pass inmapToTypedAnyWithOptionsto parse string → bool/int/uint/float based on the target protobuf field kind (dropping empty-string scalars to preserve proto3 defaults). - Implemented
coerceStringToScalarto handle the supported coercions and intentionally pass through unparseable strings soprotojsonsurfaces the canonical error. - Added unit + regression tests covering coercion behavior, keying by proto vs JSON name, nil/empty guards, and the
"false"-for-bool template regression.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| plugin/external/convert.go | Adds scalar string coercion before STRICT_PROTO protojson decoding in mapToTypedAnyWithOptions. |
| plugin/external/coerce_test.go | Adds focused tests for scalar coercion and a regression test for template-emitted "false" on bool fields. |
Comment on lines
+149
to
+153
| } | ||
| out := make(map[string]any, len(values)) | ||
| for k, v := range values { | ||
| out[k] = v | ||
| field, ok := fieldByKey[k] |
Comment on lines
+255
to
+257
| // Ensure unused import in test stays referenced (anypb is reachable via | ||
| // mapToTypedAny in TestMapToTypedAny_*). | ||
| var _ = anypb.Any{} |
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
coerceMapScalarsinplugin/external/convert.goto pre-parse string-typed values into Go bool/int/uint/float before the protojson decoder rejects them.step.auth_methods_policyconfig: workflow template expansion via{{ config "password_auth_enabled" }}emits the string"false", which strict-proto's protojson decoder rejected withinvalid value for bool field passwordAuthEnabled: "false".Why
Workflow template (
{{ config "..." }}) always emits string output even when the underlying config default is a scalar. Engine v0.51.x strict_proto routes typed configs throughprotojson.UnmarshalOptions{DiscardUnknown: false}which is strict on field types — so every template-driven scalar field broke at engine build time without this coercion.Without coercion, the fix would have to be replicated by every plugin (publish a coerce-aware contract) or every consumer (rewrite every template + YAML to dodge the issue per field type) — neither is sustainable. Coercing at the gateway keeps the contract clean.
Coverage
true/false/1/0/yes/no/on/off(case-insensitive)strconv.ParseInt(value, 10, 64)strconv.ParseUint(value, 10, 64)strconv.ParseFloat(value, 64)Copy-on-coerce; caller's map is not mutated.
Test plan
go test ./plugin/external/... -run Coerce|StringFalse— 9 new tests passgo test ./plugin/external/...— green🤖 Generated with Claude Code