fix(completion): treat boolean inputs as booleans, not quoted strings - #283
X-Guardian wants to merge 2 commits into
Conversation
eFAILution
left a comment
There was a problem hiding this comment.
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))
endBaseInput.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. (.inf → Infinity 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 comparesparam.default === value. That holds after the backfill, but a catalog parameter with no matching template entry keeps its API-typed default, so abooleaninput 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 invalues.- A non-required
booleanwith 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. parseScalarsends every default and every option throughparseYaml, which memoises into the same 50-entry module cache the real CI-file parses use, one entry perprobe: x. Parsing one spec can evict the document parses it sits next to.yaml.loadwithGITLAB_CI_SCHEMAdirectly skips the cache.npm testreports 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 baredefault:at four spaces, so a spec indentedinputs:→ 2 → 4 produces a phantom input nameddefault. The test only exercises the six-space form.
|
Review addressed. 1. Untyped inputs are
|
| 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. .inf → Infinity 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
parseYamlcache — good catch, switched toyaml.loadwithGITLAB_CI_SCHEMAdirectly. These probes are one-off strings, so memoising them was pure eviction pressure on the document parses sharing that cache.- Phantom
defaultinput — 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 baredefault: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,true→true,falsefor 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.
Summary
booleaninput inserted its default quoted —test: "false"where GitLab expects the booleanfalse— offered notrue/falsedropdown, and suggested nothing at all in the value slot (cursor aftertest:).options:list that a boolean never has.specParser.tsreads the spec line-by-line and assigneddefault:by slicing raw text, sodefault: falsebecame the string"false".buildInputInsertValuethen correctly concluded a string"false"needs quoting to survive a round-trip — it was handed the wrong type, not doing the wrong thing.test: "false") and no true/false dropdown is offered #282Change Type
Context
User-facing impact
test: false, nottest: "false".true/falsedropdown in both completion slots — when adding the input, and when typing its value. Values are listedtruebeforefalse; the snippet pre-selects a declared default, and the value-slot dropdown labels it(default).default: 8080inserts as8080, not"8080".type:are typed by their default, as GitLab does, so an untypeddefault: falsebehaves as the boolean it is.JSON.stringify, so it inserted"production"for a plain string default too.The four layers
Each was individually sufficient to keep the bug visible, which is why partial fixes didn't clear it:
specParsersliceddefault:as texttest: "false"— a string where a boolean is declaredtype:fell back to'string'when omitteddefault: falsegets no dropdown; completion can't tell it's a booleanoptions:listtest:— a boolean has nooptions:Why this survived #169
It touched the same function, added the
options:choice, and made string insertion unquoted. But it added theboolean → ${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:
validationProviderquoted it viaJSON.stringifyin both the missing-input picker and the quick-fix insert, and anoptions: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 withisParameterDefault, so it was correct throughout — this only ever affected remote/catalog components, which is likely why it went unreported.GitLab scope
Affected areas
What changed by bucket
src/parsers/specParser.tsparseDefaultValueround-trips the text afterdefault:through the YAML parser, sofalseis a boolean and a quoted"false"stays a string. An explicittype: stringkeeps the value literal, sodefault: 1.0stays"1.0"and0755keeps its leading zero. Becausetype:may followdefault:, the default is resolved once the input is complete (finalizeInput). Applied to the legacyspec.variablesfallback too.ComponentVariable.defaultwidened fromstringtoParameterDefault.options:kept in stepsrc/parsers/specParser.tsoptions:entries are typed by the sameparseScalar, andComponentVariable.optionswidened fromstring[]toArray<string | number | boolean>(matchingComponentParameter.options, which was already the wider type). Typing defaults while leaving options as strings would mean adefault: falsenever matched thefalseinoptions: [true, false]— the default wouldn't pre-select, and the choice would insert"true"/"false"into a boolean input. Entries are resolved infinalizeInputfor the same reason defaults are:type:may be declared afteroptions:.src/parsers/specParser.tsinferTypeFromDefault: an input that declares a default but notype:takes its type from the default, mirroring GitLab, instead of the'string'fallback.src/services/component/componentFetcherTemplates.tsbackfillParameterOptions→backfillParameterSpecDetail, now graftingtypeanddefaultfrom the parsed template as well asoptions. An explicit catalog type still wins; only the'string'fallback is filled in. Renamed because it no longer backfills just options.src/providers/completionInputContext.ts,src/providers/completionProvider.tsallowedValuesFor— an explicitoptions:list, ortrue, falsefor aboolean. Both completion slots use it, so the input-name snippet and the value-slot dropdown can't drift apart again. The unreachablecase 'boolean'in the type switch is removed.src/providers/completionInputContext.ts,src/providers/completionProvider.tstruebeforefalse— 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 itsdetailtext instead, so the list doesn't reshuffle per input.src/providers/completionInputContext.ts,src/providers/validationProvider.tsrenderDefaultValue, reused invalidationProviderto replace twoJSON.stringifycall sites that quoted every scalar.src/types/git-component.ts,src/providers/localComponentResolver.tsisParameterDefaultmoved next to theParameterDefaulttype it narrows and exported, rather than duplicated; the resolver's private copy is deleted.src/services/cache/componentCacheManager.tsCURRENT_CACHE_VERSION1.3.0→1.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 compilenpm 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 test notes
Parsed specs through the real
GitLabSpecParserand fed the results to the realbuildInputInsertValue:testtype: boolean,default: falsetest: "false"test: ${1|false,true|}untyped_booldefault: false(no type)"false", typestring${1|false,true|}, typebooleanrequired_flagtype: boolean, no default${1|true,false|}literal_falsetype: string,default: "false""false"porttype: number,default: 8080"8080"8080ratiotype: string,default: 1.0"1.0""1.0"— not renumbered to1envtype: string,default: productionproductiontrickytype: string,default: "key: value""key: value"listytype: array,default: [a, b][a, b]The
literal_falseandratiorows are the regression guards: an explicitly quoted"false"is a real string and must stay quoted, and a declared-string1.0must 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,findCompletionInputContextAtLinereportsslot: 'value'andallowedValuesForreturns[false, true]— previously the provider returnednullhere.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 recovertype: 'boolean'and a realfalse, while a plain string input is untouched.Tests
GitLabSpecParser— newdefault value typessuite: scalar defaults parse to their YAML types; a quoted"false"/"8080"stays a string; an emptydefault:is''and leaves the input optional; afalsedefault marks the input optional; omitted types are inferred from the default; an explicittype: stringkeeps1.0/0755as text;options:entries are typed like defaults (so[true, false]are booleans and afalsedefault pre-selects), including whentype:is declared after the options block.buildInputInsertValue/allowedValuesFor— booleans offer the choice with a default pre-selected; a boolean choice is never quoted;allowedValuesForcovers the value slot and returnsundefinedfor 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.GitLabSpecParser.test.tsasserteddebug.default === 'false'(nowfalse), andcompletionInputContext.test.tsasserted a boolean default rendered as bare'true'(now a choice). Both were codifying the bug.Breaking Changes
The cache-version bump discards existing on-disk component caches on first load after upgrade; they are refetched transparently. No user action required.
ComponentVariable.defaultwidening fromstringtoParameterDefaultis a compile-time change to an internal parser type; all consumers already declared the widerParameterDefaultand were receiving strings at runtime in violation of their own annotations.tsc --noEmitis clean across bothsrcand the test project.Screenshots / Recordings (if UI behavior changed)
testinsertstest: "false"; no dropdown; completion aftertest:offers nothing.testinsertstest: falsewith afalse/truedropdown, and completion aftertest:offers both values.Risk and Rollback
parseDefaultValuenow 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_SCHEMAkeepsyes/no/on/offas strings, an explicittype: stringopts out entirely, and the unparseable case falls back to literal text, but this is the surface to watch; (3) inputs that previously reportedtype: '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.Release Notes Draft
test: false, nottest: "false") and offer a true/false dropdown both when adding the input and when filling in its value.Checklist