Fix concrete execution divergences from abstract shell

Arran Walkerrequested to merge
ajwalker/concrete-coverage-fixes into main

What does this MR do?

Resolves 18 behavior divergences between concrete execution (FF_CONCRETE) and the abstract-shell path. Each commit fixes one self-contained divergence and is backed by a unit test, a backbench integration test, or both. None of these change behavior when FF_CONCRETE is off.

Why is this needed?

Concrete's builder/runtime had drifted from the abstract shell in a handful of subtle ways — feature flags that silently no-op'd, sanitizer passes that ran in one path but not the other, and config defaults that disagreed. Most are invisible on the happy path but surface as correctness or security regressions for users who toggle the flag.

Changes by area

Cache

  • Wire cache alternate URL/file/HEAD URL through concrete — the alternate-bucket fallback was unwired in concrete; primary URL failures couldn't fall through to the alternate.
  • Reject unknown cache policy values — a typo in cache.policy (or in $CACHE_POLICY after expansion) used to silently disable caching; now surfaces as a hard error.
  • Apply CACHE_FALLBACK_KEY -protected check after expansion — the suffix gate ran against the literal key string, not the variable-expanded result.

Get sources / git

  • Preserve .git config under GIT_STRATEGY=none — concrete wiped the existing .git directory; abstract leaves it alone.
  • Honor GIT_SUBMODULE_UPDATE_FLAGS=none — the sentinel value was passed verbatim to git submodule update instead of suppressing the flag list.
  • Probe git version via the bundled binary — gitVersionAtLeast shelled out to git from PATH, but concrete's runtime in the build container has no git on PATH, only the bundled binary. This silently killed the FF_USE_GIT_NATIVE_CLONE opt-in.
  • Gate Gitaly correlation ID header on FF_USE_GITALY_CORRELATION_ID — always-on in concrete, FF-gated in abstract.

Artifacts

  • Sanitize repo URL before SLSA artifact metadata — concrete passed b.meta.GitInfo.RepoURL verbatim into the SLSA provenance JSON, leaking the gitlab-ci-token: userinfo segment that GitLab embeds in the URL. Abstract calls RepoCleanURL() first.

Feature flag wiring

  • Honor FF_USE_EXPONENTIAL_BACKOFF_STAGE_RETRY across concrete retry loops (get_sources, cache_extract, artifact_download).
  • Honor FF_USE_NEW_BASH_EVAL_STRATEGY in the concrete scriptwriter.
  • Thread FF_USE_NEW_BASH_EVAL_STRATEGY and FF_SCRIPT_SECTIONS into concrete clone hooks (pre/post clone steps).

Builder configuration

  • Read repo object format from GitInfo instead of the GIT_OBJECT_FORMAT CI variable that GitLab never sets.
  • Reject negative RUNNER_*_TIMEOUT durations — abstract clamps to default, concrete used to accept and propagate the negative value.

Variable expansion / env

  • Honor $$ escape and shell special params in ExpandValue — $$ now resolves to a literal $, and shell-special params ($*, $#, $@, $!, $?, $-, $0..$9) resolve to empty rather than leaking same-named env entries.
  • Reload GITLAB_ENV between concrete prepare sub-stages — entries written by get_sources / restore_cache / download_artifacts were not picked up by subsequent prepare-time stages.

Lifecycle

  • Refresh env status after after_script promotes an error — the post-after_script status reload was missing, so subsequent artifact selection saw stale state.
  • Stop swallowing script exit code on step AllowFailure — concrete's Step.Run treated allow_failure: true as "ignore failure entirely", dropping the script's exit code. Abstract shell ignores AllowFailure on script steps too — concrete now matches: the step failure propagates, the exit code reaches the runner core, and failure_reason is set to script_failure. Per-step allow_failure remains a runner-wide gap (no path implements it).

Cleanup

  • Trim over-explaining inline comments in concrete code — removed comments that described removed/previous behavior or referenced abstract-shell line numbers.

How were these found?

Mostly via a fine-tooth comb against the abstract shell, plus a coverage-driven sweep through functions/concrete/run/stages / functions/concrete/builder that surfaced previously-dark code paths (the bundled-git probe was the last one, found exactly because the gitClone branch sat at 0% coverage despite the FF being on).

Backbench coverage for functions/concrete/run rose from 66.8% to 84.9% over the course of this work, and functions/concrete/run/stages from 52.7% to 66.1%.

Risk

Each commit is scoped to one divergence with targeted tests. With FF_CONCRETE off, this MR is a no-op. With FF_CONCRETE on, the changes move concrete closer to abstract-shell semantics — the failure modes they replace are mostly silent (FF flags that didn't take effect, sanitizers that didn't run), so existing concrete users should see either no change or stricter behavior they would have wanted anyway.

Testing

  • Unit tests added/updated per commit (functions/concrete/...).
  • Backbench bench tests in companion changes: artifact_metadata.yaml (SLSA fix), plus new fixtures for artifact-download, run-keyword, feature-flag-on-bash-script, expand-value, git-strategy/submodules.
  • After the AllowFailure fix, the three allow_failure/* bench tests fail identically on baseline and concrete (both surface exit_code + script_failure), where previously concrete silently zeroed the exit code. The tests continue to document the runner-wide allow_failure gap.

Closes Closes #39473 (closed)

Edited by Arran Walker

Merge request reports