Skip to content

fix(completion): treat boolean inputs as booleans, not quoted strings - #283

Open
X-Guardian wants to merge 2 commits into
eFAILution:betafrom
X-Guardian:fix/boolean-input-completion
Open

X-Guardian wants to merge 2 commits into
eFAILution:betafrom
X-Guardian:fix/boolean-input-completion

Conversation

@X-Guardian

Copy link
Copy Markdown
Contributor

Summary

  • Completing a boolean input inserted its default quotedtest: "false" where GitLab expects the boolean false — offered no true/false dropdown, and suggested nothing at all in the value slot (cursor after test:).
  • These are four defects on one path, and each had to be fixed for the symptom to clear. The one users hit first is the last one: the two completion slots didn't share a notion of what values an input may take, so the value slot gated on a literal options: list that a boolean never has.
  • Root cause of the quoting is the spec parser, not the completion logic: specParser.ts reads the spec line-by-line and assigned default: by slicing raw text, so default: false became the string "false". buildInputInsertValue then correctly concluded a string "false" needs quoting to survive a round-trip — it was handed the wrong type, not doing the wrong thing.
  • Link related issue(s): Fixes bug: completion: boolean input default is inserted quoted (test: "false") and no true/false dropdown is offered #282

Change Type

  • feat
  • fix
  • refactor
  • docs
  • test
  • chore

Context

User-facing impact

  • A boolean default inserts unquoted: test: false, not test: "false".
  • Booleans offer a true/false dropdown in both completion slots — when adding the input, and when typing its value. Values are listed true before false; the snippet pre-selects a declared default, and the value-slot dropdown labels it (default).
  • Numeric and null defaults are fixed by the same change: default: 8080 inserts as 8080, not "8080".
  • Inputs that omit type: are typed by their default, as GitLab does, so an untyped default: false behaves as the boolean it is.
  • The validation quick-fix stops quoting every default. "Add missing inputs" rendered defaults with JSON.stringify, so it inserted "production" for a plain string default too.
  • On-disk caches are discarded once on upgrade and transparently refetched.

The four layers

Each was individually sufficient to keep the bug visible, which is why partial fixes didn't clear it:

# Layer Symptom if left unfixed
1 specParser sliced default: as text test: "false" — a string where a boolean is declared
2 type: fell back to 'string' when omitted An untyped default: false gets no dropdown; completion can't tell it's a boolean
3 Catalog path discarded the parsed type Catalog-resolved components get no dropdown even with 1 and 2 fixed
4 Value slot required a literal options: list No suggestions at all after test: — a boolean has no options:

Why this survived #169

It touched the same function, added the options: choice, and made string insertion unquoted. But it added the boolean → ${1|true,false|} branch only on the no-default path and never touched the parser, so any boolean with a default still produced "false". It also only ever addressed the input-name slot.

Why the fix belongs in the parser

The stringified default leaks into several consumers, so fixing it at the completion boundary would leave the others wrong: validationProvider quoted it via JSON.stringify in both the missing-input picker and the quick-fix insert, and an options: list of real booleans never matched a "false" default, so the default didn't float to the front.

The local: include path (localComponentResolver.ts) already parsed real YAML and guarded with isParameterDefault, so it was correct throughout — this only ever affected remote/catalog components, which is likely why it went unreported.

GitLab scope

  • gitlab.com
  • self-managed GitLab
  • both (parsing and snippet logic are instance-independent)

Affected areas

  • Component Browser
  • Hover provider
  • Completion provider
  • Validation provider (missing-input picker and quick-fix insert)
  • Cache and refresh behavior (cache-version bump only)
  • GitLab API calls/auth/token storage
  • Docs only

What changed by bucket

Bucket Files Approach
The root cause src/parsers/specParser.ts New parseDefaultValue round-trips the text after default: through the YAML parser, so false is a boolean and a quoted "false" stays a string. An explicit type: string keeps the value literal, so default: 1.0 stays "1.0" and 0755 keeps its leading zero. Because type: may follow default:, the default is resolved once the input is complete (finalizeInput). Applied to the legacy spec.variables fallback too. ComponentVariable.default widened from string to ParameterDefault.
options: kept in step src/parsers/specParser.ts options: entries are typed by the same parseScalar, and ComponentVariable.options widened from string[] to Array<string | number | boolean> (matching ComponentParameter.options, which was already the wider type). Typing defaults while leaving options as strings would mean a default: false never matched the false in options: [true, false] — the default wouldn't pre-select, and the choice would insert "true"/"false" into a boolean input. Entries are resolved in finalizeInput for the same reason defaults are: type: may be declared after options:.
Inferred types src/parsers/specParser.ts New inferTypeFromDefault: an input that declares a default but no type: takes its type from the default, mirroring GitLab, instead of the 'string' fallback.
Catalog path src/services/component/componentFetcherTemplates.ts backfillParameterOptionsbackfillParameterSpecDetail, now grafting type and default from the parsed template as well as options. An explicit catalog type still wins; only the 'string' fallback is filled in. Renamed because it no longer backfills just options.
One set of allowed values src/providers/completionInputContext.ts, src/providers/completionProvider.ts New exported allowedValuesFor — an explicit options: list, or true, false for a boolean. Both completion slots use it, so the input-name snippet and the value-slot dropdown can't drift apart again. The unreachable case 'boolean' in the type switch is removed.
Ordering and the default src/providers/completionInputContext.ts, src/providers/completionProvider.ts Booleans list true before false — conventional reading order for a list someone is scanning. The snippet still floats a declared default to the front (VS Code pre-selects the first choice entry), while the value-slot dropdown keeps a stable order and marks the default in its detail text instead, so the list doesn't reshuffle per input.
One value renderer src/providers/completionInputContext.ts, src/providers/validationProvider.ts Default-to-YAML rendering extracted as the exported renderDefaultValue, reused in validationProvider to replace two JSON.stringify call sites that quoted every scalar.
Shared guard src/types/git-component.ts, src/providers/localComponentResolver.ts isParameterDefault moved next to the ParameterDefault type it narrows and exported, rather than duplicated; the resolver's private copy is deleted.
Cache invalidation src/services/cache/componentCacheManager.ts CURRENT_CACHE_VERSION 1.3.01.4.0. Stored defaults change type and inputs gain more accurate types, so the bump comment now notes that value-type changes count too.

Validation

Local checks

  • npm run compile
  • npm test (421 passing)
  • npm run lint (clean, --max-warnings 0)
  • npm run test:extension-host (25 passing — exercises the real completion provider inside VS Code, value slot included)
  • Manual verification in VS Code Extension Host

Manual test notes

Parsed specs through the real GitLabSpecParser and fed the results to the real buildInputInsertValue:

Input Spec Before After
test type: boolean, default: false test: "false" test: ${1|false,true|}
untyped_bool default: false (no type) "false", type string ${1|false,true|}, type boolean
required_flag type: boolean, no default ${1|true,false|} unchanged
literal_false type: string, default: "false" "false" unchanged — genuinely a string
port type: number, default: 8080 "8080" 8080
ratio type: string, default: 1.0 "1.0" "1.0" — not renumbered to 1
env type: string, default: production production unchanged
tricky type: string, default: "key: value" "key: value" unchanged
listy type: array, default: [a, b] [a, b] unchanged

The literal_false and ratio rows are the regression guards: an explicitly quoted "false" is a real string and must stay quoted, and a declared-string 1.0 must not be renumbered. #169's quoting behaviour for hazardous strings is untouched.

Value slot verified separately: with the line test: and the cursor after the colon, findCompletionInputContextAtLine reports slot: 'value' and allowedValuesFor returns [false, true] — previously the provider returned null here.

Catalog path verified by feeding catalog-shaped parameters (type-poor, stringified defaults) plus the parsed template through backfillParameterSpecDetail: both a typed and an untyped boolean recover type: 'boolean' and a real false, while a plain string input is untouched.

Tests

  • GitLabSpecParser — new default value types suite: scalar defaults parse to their YAML types; a quoted "false"/"8080" stays a string; an empty default: is '' and leaves the input optional; a false default marks the input optional; omitted types are inferred from the default; an explicit type: string keeps 1.0/0755 as text; options: entries are typed like defaults (so [true, false] are booleans and a false default pre-selects), including when type: is declared after the options block.
  • buildInputInsertValue / allowedValuesFor — booleans offer the choice with a default pre-selected; a boolean choice is never quoted; allowedValuesFor covers the value slot and returns undefined for free-text inputs.
  • backfillParameterSpecDetail — recovers a boolean type the catalog reported as an untyped string, keeps an explicit catalog type over the template's inference, and leaves a catalog string default alone.
  • Two existing assertions encoded the old behaviour and are updated: GitLabSpecParser.test.ts asserted debug.default === 'false' (now false), and completionInputContext.test.ts asserted a boolean default rendered as bare 'true' (now a choice). Both were codifying the bug.

Breaking Changes

  • No breaking changes
  • Breaking changes (describe below)

The cache-version bump discards existing on-disk component caches on first load after upgrade; they are refetched transparently. No user action required.

ComponentVariable.default widening from string to ParameterDefault is a compile-time change to an internal parser type; all consumers already declared the wider ParameterDefault and were receiving strings at runtime in violation of their own annotations. tsc --noEmit is clean across both src and the test project.

Screenshots / Recordings (if UI behavior changed)

  • Before: accepting test inserts test: "false"; no dropdown; completion after test: offers nothing.
  • After: accepting test inserts test: false with a false/true dropdown, and completion after test: offers both values.

Risk and Rollback

  • Main risks: (1) the cache-version bump triggers a one-time refetch for all users; (2) parseDefaultValue now interprets a default it previously passed through verbatim — a value that looks like YAML but was meant literally (default: yes, default: null) changes type on a non-string input. GITLAB_CI_SCHEMA keeps yes/no/on/off as strings, an explicit type: string opts out entirely, and the unparseable case falls back to literal text, but this is the surface to watch; (3) inputs that previously reported type: 'string' may now report a more specific inferred type, which other consumers read; (4) booleans now insert a snippet choice rather than plain text, so accepting one leaves the cursor in a choice placeholder.
  • Rollback strategy: revert the commit. Any change to the cache-version constant re-discards caches, so rollback self-heals the cache shape.

Release Notes Draft

  • Boolean inputs now complete as real booleans (test: false, not test: "false") and offer a true/false dropdown both when adding the input and when filling in its value.

Checklist

  • Branch is up to date with target branch
  • Commit messages follow conventional commits
  • Added/updated docs for behavior or settings changes
  • Added/updated tests for new behavior
  • No secrets or tokens in code, logs, screenshots, or test fixtures

@eFAILution eFAILution left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diagnosis is right where it counts: this is a parser bug, not a snippet-builder bug, and buildInputInsertValue was doing the correct thing with the wrong input. allowedValuesFor as one source of truth for both slots is the right shape, and typing options: entries in the same pass so a false default can match the false in the list is the detail most fixes would miss.

Verified locally on b34e0b9: npm test 422 passing, eslint --max-warnings 0 clean, tsc --noEmit clean.

Requesting changes on one premise, which turns out to corrupt values.

Before merge

1. GitLab does not infer an input's type from its default — an omitted type: is string.

Three things rest on this: inferTypeFromDefault, the decision in parseDefaultValue to run the YAML parser whenever the declared type isn't string, and the "as GitLab does" claim in the body. GitLab's own resolver says otherwise:

# lib/ci/inputs/string_input.rb
def self.matches?(spec)
  # The input spec can be `nil` when using a minimal specification
  # and also when `type` is not specified.
  spec.nil? || super || (spec.is_a?(Hash) && !spec.key?(:type))
end

BaseInput.matches? is spec.is_a?(Hash) && spec[:type] == type_name, and Builder picks the first class whose matches? returns true — so an untyped spec matches only StringInput, whose coerced_value is value.to_s. The docs agree: type is "string (default when not specified)".

The consequence is that the guard keeping 1.0 and 0755 literal sits on the type: string branch — the branch that needed it least — while the untyped branch, which GitLab also treats as string, now goes through YAML. Fed these through this PR's own GitLabSpecParser:

untyped spec before this PR GitLab
default: 0755 0755 755 "0755"
default: 007 007 7 "007"
default: 1.0 1.0 1 "1.0"
default: 0123456789 0123456789 123456789 "0123456789"
default: 1e5 1e5 100000 "1e5"
default: .inf .inf Infinity ".inf"

So a component declaring file_mode: with default: 0755 and no type: now completes to file_mode: 755, and a version: with default: 1.0 completes to 1. That's a silent value change, which is worse than the quoting bug it ships alongside. (.infInfinity isn't even valid YAML for infinity, so it round-trips as a string.)

The fix is small: treat an omitted type as string, i.e. pass declaredType ?? 'string' into parseDefaultValue, and drop inferTypeFromDefault. #282's headline case — a declared type: boolean — still clears. What's lost is the untyped-boolean sub-case, which the issue asserts but GitLab doesn't do.

If you'd rather keep the inference as a deliberate deviation — the argument that someone writing default: false meant a boolean whatever GitLab coerces it to is not a bad one — then say that in the comment and the body instead of "as GitLab does", and narrow it to booleans so numeric and zero-padded defaults aren't renumbered.

2. backfillParameterSpecDetail does the opposite of what the body says.

The body says "An explicit catalog type still wins; only the 'string' fallback is filled in", and the test list says it "keeps an explicit catalog type over the template's inference". The code is:

...(fromTemplate.type ? { type: fromTemplate.type } : {}),

GitLabSpecParser always sets type — the 'string' fallback included — so fromTemplate.type is always truthy and the catalog's type is always overwritten, by the parser's fallback as readily as by a real parse. The tests agree with the code ("the local parse wins on the type signature"), so it's the body that's stale; but the two readings differ in behaviour. A catalog input the API types number, whose type: line our line-based parser doesn't pick up, is now downgraded to string — a path that didn't exist when only options was grafted.

Pick one and make code, tests and body agree. If the tests have it right, the body needs correcting in two places; if the body has it right, the condition needs to be "only when the catalog left the fallback".

3. parseScalar carries parseDefaultValue's doc comment verbatim.

src/parsers/specParser.ts — the block above parseScalar is a copy of the one above parseDefaultValue, @param rawValue and all, while the function's parameter is trimmed and it takes no declaredType. Two adjacent functions with identical docs points the next reader at the wrong one.

Smaller things

  • completionProvider.ts — the (default) label compares param.default === value. That holds after the backfill, but a catalog parameter with no matching template entry keeps its API-typed default, so a boolean input whose catalog default arrives as the string 'false' renders ${1|"false",true,false|} — a three-entry choice whose first entry is a quoted string. Reachable whenever the template parse yields no parameters at all, since the backfill is then skipped entirely. Worth comparing type-aware, or dropping a default that isn't in values.
  • A non-required boolean with no default used to insert ${1|false,true|} and now inserts ${1|true,false|}. Only reachable from catalog data, since the parser marks a defaultless input required, and the new order is the one the PR argues for — just noting the manual-test table's "unchanged" doesn't hold for that row.
  • parseScalar sends every default and every option through parseYaml, which memoises into the same 50-entry module cache the real CI-file parses use, one entry per probe: x. Parsing one spec can evict the document parses it sits next to. yaml.load with GITLAB_CI_SCHEMA directly skips the cache.
  • npm test reports 422 here, not the 421 in the body.
  • Pre-existing, but the new empty-default test reads as covering it: the input-name regex ^\s{2,4}[a-zA-Z_][a-zA-Z0-9_-]*:\s*$ also matches a bare default: at four spaces, so a spec indented inputs: → 2 → 4 produces a phantom input named default. The test only exercises the six-space form.

@X-Guardian

Copy link
Copy Markdown
Contributor Author

Review addressed.

1. Untyped inputs are string — fixed

Confirmed the corruption through this PR's own parser before changing anything:

untyped spec on b34e0b9 now
default: 0755 755 "0755"
default: 007 7 "007"
default: 1.0 1 "1.0"
default: 0123456789 123456789 "0123456789"
default: 1e5 100000 "1e5"
default: .inf Infinity ".inf"

Took the first option rather than keeping the inference as a deviation. The argument for it — someone writing default: false meant a boolean — is decent, but it only holds for booleans, and a rule that special-cases one type to avoid corrupting the rest is a worse rule than matching GitLab. .infInfinity also showed the parse was already producing values that don't round-trip, which made the inference harder to defend.

So: inferTypeFromDefault is gone, and parseDefaultValue/parseOptionEntry take the resolved type — 'string' when the spec omits it. The guard that keeps 1.0 and 0755 literal now covers the untyped branch too, which is where it was actually needed.

#282's headline case (declared type: boolean) still clears. What's lost is the untyped-boolean sub-case the issue asserted; an untyped default: false now completes to "false", matching to_s. I've corrected the issue body.

New test treats an omitted type as string, leaving the default as written pins all six rows above.

2. backfillParameterSpecDetail — the body was stale, but neither reading was right

The tests described the code, so the body was the stale half. But fixing the body to match would have locked in the regression you spotted in passing: our line-based parser always sets type, so fromTemplate.type was always truthy, and a catalog input the API types number whose type: line we don't pick up was being downgraded to string.

"Only when the catalog left the fallback" doesn't work either — v.type || 'string' makes an explicit catalog string indistinguishable from the fallback, so that condition can't be expressed on the catalog side.

Went with the symmetric version: the template's type wins only when it's more specific than the 'string' fallback both sides share. options and default still come from the template unconditionally. Code, tests and body now agree, and there's a test for the downgrade case.

3. parseScalar doc comment — fixed

Rewritten to describe parseScalar and its actual trimmed parameter.

Smaller things

  • parseYaml cache — good catch, switched to yaml.load with GITLAB_CI_SCHEMA directly. These probes are one-off strings, so memoising them was pure eviction pressure on the document parses sharing that cache.
  • Phantom default input — fixed rather than left pre-existing, since you're right that the new empty-default test reads as covering it. Input-name matching now excludes an input's own keys (description/default/type/options/regex), so a bare default: at four-space indentation no longer opens a phantom input and swallows the real one's value. Test added at that layout.
  • Test count — 424 now.
  • false,truetrue,false for a defaultless boolean — correct, and deliberate: conventional reading order for a scanned list. The manual-test table's "unchanged" on that row is wrong and I've fixed it.

One I couldn't reproduce

The ${1|"false",true,false|} case. buildInputInsertValue drops a default that isn't already among the rendered values:

const ordered =
  defaultRendered !== undefined && rendered.includes(defaultRendered)
    ? [defaultRendered, ...rendered.filter((v) => v !== defaultRendered)]
    : rendered;

A string 'false' renders as "false" (quoted), which isn't in ['true','false'], so it falls to the : rendered branch. Checked both that and a default outside its options: list:

param result
{type:'boolean', default:'false'} ${1|true,false|}
{default:'azure', options:['aws','gcp']} ${1|aws,gcp|}

The value-slot (default) label has the same property for a different reason — param.default === value just doesn't match, so no label renders, which is right for a default that isn't offered. I may be missing the path you had in mind; if the guard isn't reached some other way, point me at it and I'll fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants