fix: Validate parsed steps without re-encoding them as YAML
A job script that starts with a blank line or a tab, or whose first line is indented more than the next, does not run correctly on a stock runner with FF_SCRIPT_TO_STEP_MIGRATION on. The step request the runner writes does not decode, and a script indented on every line runs with its indentation dropped. gitlab-org/gitlab-runner!7660 makes the runner quote the strings it writes. Once it does, these scripts reach step-runner's validation, which fails on them. This MR fixes that half, so these scripts run as written once both land.
Closes #480
flowchart LR
read["ReadSteps: decode YAML into typed Spec and Step"] --> validate["validate against spec.json and step.json"]
validate --> before["before: yaml.Marshal to text, then yaml.Unmarshal to any"]
validate --> after["after: untype walks the typed values in memory"]
before -->|"script with a leading blank line, a leading tab or a more-indented first line"| fail["parse error: job fails at validation"]
before -->|"other scalars"| check["JSON schema check"]
after --> check
check --> run["Compile and run: script reaches the shell as decoded"]
fail ~~~ runStep Runner checks each parsed step against its JSON schema. To do that it wrote the step back out as YAML text and parsed the text again. go-yaml v3 writes some multi-line strings in a form it cannot read back (go-yaml issue 76), so the document had parsed and then failed validation. On main the errors are such as validating function schema: yaml: line 1: did not find expected key in the tests and, through the runner, validating function schema: yaml: line 6: did not find expected '-' indicator. The line number varies. A leading tab reports found a tab character where an indentation space is expected.
An upgrade would not help yet:
- The fix is go-yaml PR 414, merged on go-yaml's main branch on 13 October. No tagged release has it, and the newest v4 tag,
v4.0.0-rc.6, predates it. - go-yaml's README keeps v3 to security fixes, so the fix will not land there.
The change
schema/v1/validate.go:untypewalks the typed value with reflection and builds theanytree directly, with theyamltag name andomitemptyrules the encoder applied. Strings are copied as they are, sovalidateSpecandvalidateStepno longer have an error to return. Encoding to ayaml.Nodegoes through the same text, which is why the walk builds the tree itself.schema/v1/signature.go: theoutputsreification decodes the original node. It marshaled a map and parsed it again, which broke for a string in an output default.schema/v1/step.go: afuncorstepreference decodes from the original node too, so arevthat starts with a tab reads as written.TestUntypeMirrorsEveryReachableShapewalks every type reachable from a spec or step and fails if one gains a shapeuntypedoes not copy: anIsZeroor marshal method, an inline or flow tag, an embedded or unexported field, a struct withomitempty, atime.Durationor ayaml.Node.
How it was tested
TestReadPreservesScalarsreads seven script shapes. Onmainfour fail (leading blank line, first line indented more than the next, blank lines then unindented text, leading tab) and all seven pass here.TestCanRunAScriptgains an end-to-end case throughtestutil.StepRunnerthat fails onmainwith the error above.TestReadPreservesOutputDefaultscovers theoutputsreification,TestReadPreservesReferenceScalarscovers afuncand astepreference whoserevstarts with a tab, andTestUntypecompares the walk with the old YAML round trip for a spec and a step that use every field type.
The table covers the default helper, not FF_CONCRETE. Job scripts of each shape ran with gitlab-runner's step mode on a self-managed GitLab 19.5.0 instance, and the output was compared with the legacy script path, the same job with the flag off:
| Job script | Stock runner | Companion runner MR only | Both changes |
|---|---|---|---|
| Ordinary, or a tab inside a line | output equals legacy | output equals legacy | output equals legacy |
| Every line indented | runs, indentation dropped | output equals legacy | output equals legacy |
| Leading blank line | request does not decode | fails in validation | output equals legacy |
| First line indented more than the rest | request does not decode | fails in validation | output equals legacy |
| Blank lines, then unindented text | request does not decode | fails in validation | output equals legacy |
| Leading tab | request does not decode | fails in validation | output equals legacy |
gitlab-runner takes step-runner through go.mod, so it needs a release that includes this change. The CHANGELOG entry is a separate commit on this branch.
An AI coding assistant wrote this change and this description. The assistant also ran the job scripts in the table above on a test VM running a self-managed GitLab 19.5.0 instance, and ran go test ./schema/... and golangci-lint run ./schema/... on the changed tree, plus the cmd/ci, cmd/run and pkg/api/service tests after make build-individual-commands. The reference decode is covered by TestReadPreservesReferenceScalars only.