fix(config): Reject non-string script: entries instead of dropping them

Paweł Farysrequested to merge
gitlab-community/gitlab-org/ci-cd/runner-tools/glci:glci-156 into main

What this MR does and why?

A script: entry written unquoted with a colon followed by a space parses as a YAML mapping, not a string. ": " is the block-mapping indicator inside a plain scalar, and the double quotes an author writes around the line quote nothing, because they are not the first character of it:

job:
  script:
    - curl -H "Authorization: Bearer $TOKEN" https://example.com   # -> {'curl -H "Authorization': 'Bearer $TOKEN" ...'}
    - echo "Error: deployment failed"                              # -> {'echo "Error': 'deployment failed"'}

toScriptList kept strings, recursed into nested lists and returned nil for everything else, so those entries vanished. The command never ran, nothing warned, and the job reported success. GitLab does not narrow the list - it refuses the file.

The fix follows #137 (closed)'s shape: separate the value from the verdict, and keep the parse permissive so glci jobs and the TUI still work on a config lint refuses.

  • scriptListValid reports GitLab's verdict and allocates nothing.
  • flattenScriptList produces the value, flattening anchor aliases exactly as before.
  • ValidateScriptsTree turns the verdict into a ConfigErrors entry worded as GitLab words it.

The nesting bound decides the verdict and nothing else. Entry::Commands::MAX_NESTING_LEVEL is now enforced, where toScriptList recursed unbounded - but the flattened value still keeps every command. Truncating it would have reintroduced, one shape over, the exact silent drop this MR exists to end. Full descent costs nothing in complexity: yaml.v3 refuses a document nested past 10000 and rejects alias bombs, so the decoder has already walked the tree before this sees it.

Covered mounts are every Entry::Commands registration, since they are literally the same class in GitLab: job script:/before_script:/after_script:, default:before_script/after_script, services:command: at job, default: and top level, hooks:pre_get_sources_script at job and default: level, and top-level before_script:/after_script:.

Validation runs before default: inheritance, or one bad default:before_script would be reported once per inheriting job instead of once against default:. It is wired into both ParseMergedYAML and resolveConfigTree, because ParseOffline overwrites ConfigErrors and a validator wired only into the former is discarded on the path glci lint actually uses.

Verified against the lint API

Every message and every gate was checked with POST /ci/lint, dry_run: true, including the cases that must produce no message:

  • a bare top-level string is valid, unlike Entry::Stages, whose wording differs for that reason
  • ten nested arrays are valid, eleven are not
  • script: null or [] is script can't be blank from Entry::Job's presence check - a different validator this does not model, so it stays silent
  • a hooks: block with an unknown key suppresses the nested pre_get_sources_script error, because Configurable#compose! builds no children of an invalid entry
  • hidden .template jobs are not validated
  • top-level services: reports as services:service:command, with no prefix, because Root's key is nil

Three deliberate divergences

  1. Multiplicity. GitLab stops at the first problem and returns only that one; glci reports every one it finds, so a single glci lint clears the file in one pass rather than one push per problem. This matches what stages: and hooks: already do.

  2. Case. Validator#messages ends with .downcase, so GitLab lowercases the whole message including the job name (jobs:Build-Job:script becomes jobs:build-job:script). glci echoes the name as written so it can be found in the file. Pre-existing in the stages and hooks paths; the docs claimed an exact match and now say "apart from case".

  3. Over-reporting under an invalid parent. Node#compose! builds no children of an entry that fails its own rules, so GitLab reports the entry alone: a job or default: carrying an unknown key, a service that is itself invalid, and the mutually exclusive pairs trigger:/script:, run:/script:, run:/before_script:. glci reports the Commands message under all of them anyway, because it models none of those parent rules - every one of those configs lints clean today apart from this message.

    This one is load-bearing, and it is worth being explicit about why. An earlier revision of this MR did suppress the mutually exclusive shapes to match GitLab, which made glci lint return CI configuration is valid for three configs GitLab refuses outright. Naming the wrong sub-entry costs a moment's confusion; a clean lint costs a failed push, and for a fidelity tool that is the worse failure by a distance. The suppression is gone, both validators now behave the same way under an invalid parent, and the behaviour is pinned by tests so it cannot regress into silence unnoticed. It becomes correct only alongside implementing the rules that would then carry the rejection - Entry::Job/Entry::Default allowed_keys, the mutually_exclusive_keys pairs, and the Imageable/Service rules - which is filed separately.

Steps to reproduce

job:
  image: alpine:3.20
  script:
    - echo "start"
    - curl -H "Authorization: Bearer $TOKEN" https://example.com
    - echo "Error: deployment failed"
    - echo "no colon space here"
    - echo "ratio 1:2 is fine"
    - echo "end"

Before, glci lint reported CI configuration is valid: 1 jobs, 5 stages and glci run passed having executed four of the six lines, with the curl and the echo "Error: ..." absent from the trace.

After:

$ glci lint
CI configuration is invalid:
  jobs:job:script config should be a string or a nested array of strings up to 10 levels deep

Quoting the line clears it, and the neighbouring shapes stay valid: echo "ratio 1:2 is fine" needs no quoting because the colon is not followed by a space, and an anchor alias still flattens rather than being rejected.

Note glci merged prints the dropped line either way - it renders the resolved YAML tree, not the parsed script list - so it cannot be used to check this. glci lint and the job trace can.

On main this surfaces through glci lint only. Nothing gates glci run on ConfigErrors yet, so a pipeline started without linting first still executes the job with the line missing. That is true of the existing stages: and hooks: diagnostics too. !174 (merged) adds ConfigRejectErrors, which reads ConfigErrors first, so this fix picks the run refusal up automatically once that merges - no rework either way. The troubleshooting page states the limitation rather than implying it is covered.

Relevant issues and other links

Edited by Paweł Farys

Merge request reports