diff --git a/SPEC.md b/SPEC.md index 496ef4972a..89b0a2b9dc 100644 --- a/SPEC.md +++ b/SPEC.md @@ -2216,14 +2216,24 @@ arrival and a live run only proves it can say yes; `scripts/test-check-fixture-execution.rb` crafts the all-six state and every absence case, and is what proves it can say no. -The roster below is still RESTATED, not derived: nothing checks it against the -runners it claims to summarise (#736). The bullets are an enumeration and the -classification on each line is judgement; both are hand-maintained, so a skip -added to a runner without a line here goes unrecorded. The manifests above are -now the input that makes it checkable for set equality against what the runners -actually reported — which is stronger than parsing -their source, and is why #736 waits for it rather than being fixed on its own. - +The roster below is HALF checked, and the split is the point. Its ENUMERATION — +which runner skips which case — is compared for set equality against the +execution manifests by `make check-fixture-execution` (#736), so a skip added to +a runner without a line here, or a line left behind after a gap closes, fails +the build. Its CLASSIFICATION and reasoning are judgement, and nothing asserts +them; that is why the section keeps its `[manual]` tag. + +The check found this roster already wrong on arrival: Kotlin and Swift each +exclude the `link-header` case wholesale through their tag branch, and the +roster described that in prose instead of enumerating it — two of six runners +misstated, in a roster whose own text promises one line per runner × test. + +Checking the enumeration against what the runners REPORTED is stronger than +parsing their source: a source parser cannot see a tag branch, a derived table, +or a case the loader dropped, which is why the enumeration waited for the +manifests rather than being checked on its own. + + **Go** (`conformance/runner/go/main.go` `goSDKSkips`) — architectural; same-origin logic is covered by `TestIsSameOrigin` unit tests: - "Mixed-case host and explicit default port stay on the mocked origin" — Go runner dials `configOverrides.baseUrl` directly; its `httptest` mock owns its origin, so origin-interception normalization does not apply. @@ -2249,11 +2259,15 @@ suppressed. **TypeScript** (`conformance/runner/typescript/runner.test.ts` `TS_SDK_SKIPS`): - "Large integer IDs preserved without precision loss" — `Number` is 53-bit (waiver 1B.6). -**Kotlin** (`kotlin/conformance/.../Main.kt` — `KOTLIN_SKIPS` is empty) — none -beyond the whole-case `link-header` tag branch described above. +**Kotlin** (`kotlin/conformance/.../Main.kt` — `KOTLIN_SKIPS` is empty; the +entry below comes from the `link-header` tag branch) — architectural: +- "List operation returns first page with Link header" — the SDK auto-paginates, and its status model reports the last consumed response, so a one-response queue yields "no response" (see the tag-branch discussion above). **Swift** (`conformance/runner/swift/.../Runner.swift` — `temporarySkips` is -empty) — none beyond the whole-case `link-header` tag branch described above. +empty; the entry below comes from the `link-header` tag branch) — architectural: +- "List operation returns first page with Link header" — same as Kotlin: auto-pagination plus a last-consumed-response status model. + + Swift carries no capability skips. It is three-gate on retry (status, network, idempotent POST) and, since #563, retries the authenticated download hop, so diff --git a/scripts/check-fixture-execution.rb b/scripts/check-fixture-execution.rb index 7abec710da..90bd5db164 100755 --- a/scripts/check-fixture-execution.rb +++ b/scripts/check-fixture-execution.rb @@ -175,6 +175,177 @@ def load_all manifests end +# --- SPEC section 19 Zero-Skip roster (#736) --------------------------------- +# +# The roster claims, in its own text, to enumerate "every skip a default +# (mock-mode) conformance run reports, one line per runner x test, verbatim +# from the runners' skip mechanisms". Nothing checked that, and it was already +# untrue when this was written: Kotlin and Swift each exclude the `link-header` +# case wholesale via their tag branch, and the roster described that in prose +# instead of enumerating it -- two of six runners wrong, in a roster nobody +# re-derives by hand. +# +# The manifests make the ENUMERATION derivable, so it is now checked for set +# equality. The classification and reasoning on each line stay judgement, and +# nothing asserts them; that half is why the section keeps its `[manual]` tag. +# +# WHY THIS PARSER IS ACCEPTABLE WHERE THE ROSTER-TABLE ONE WAS NOT. #740 +# declined to keep teaching `sync-doc-constants.rb` more GFM -- separator +# widths, backslash parity -- because a mis-parse there is SILENT: it validates +# the wrong cell and reports success. This extraction fails LOUD in both +# directions. A bullet it cannot read is a name missing from the roster set, +# which is a mismatch; a name it invents is an extra, also a mismatch. There is +# no reading of a malformed line that produces a passing comparison, so the +# failure mode is a false alarm the author fixes, never a false green. +# +# The delimiters are deliberately NOT `@`-markers. sync-doc-constants owns +# those, and it runs in spec-gates where no conformance run has happened, so it +# has no manifests to compare against. Registering a kind there whose real +# enforcement lives here would split one check across two gates. +ROSTER_BEGIN = "" +ROSTER_END = "" + +# Heading label => runner id. Hardcoded for the same reason EXPECTED_RUNNERS is: +# deriving it from whatever headings appear lets a renamed section silently drop +# a runner out of the comparison. +ROSTER_HEADINGS = { + "Go" => "go", "Python" => "python", "Ruby" => "ruby", + "TypeScript" => "typescript", "Kotlin" => "kotlin", "Swift" => "swift" +}.freeze + +# Returns { runner => [case name] } as the roster states it. +def parse_roster(spec_path) + text = File.read(spec_path, encoding: "UTF-8") + + # EXACTLY one of each delimiter. Taking the first occurrence of each would + # silently ignore a second, complete roster block — and a stale line living in + # that duplicate would never be compared while the gate reported success. That + # is a SILENT pass, which is the one failure mode this extractor is not + # allowed to have: the whole argument for parsing prose here is that every + # misreading surfaces as a set mismatch. Copilot found it. + begins = text.scan(ROSTER_BEGIN).length + ends = text.scan(ROSTER_END).length + + # No roster at all is its own failure, and a distinct one: the roster was + # deleted or renamed, not duplicated. Kept separate so the message names what + # actually happened. + i = text.index(ROSTER_BEGIN) + j = text.index(ROSTER_END) + raise Failure, "SPEC.md holds no #{ROSTER_BEGIN} / #{ROSTER_END} pair" if i.nil? || j.nil? || j < i + + if begins > 1 || ends > 1 + raise Failure, "SPEC.md must hold exactly one #{ROSTER_BEGIN} and one #{ROSTER_END}; " \ + "found #{begins} and #{ends}. A second block is not compared, so a stale " \ + "line inside it would pass unnoticed." + end + + body = text[(i + ROSTER_BEGIN.length)...j] + roster = Hash.new { |h, k| h[k] = [] } + runner = nil + seen = [] + + body.each_line do |line| + if (m = line.match(/\A\*\*([A-Za-z]+)\*\*/)) + label = m[1] + runner = ROSTER_HEADINGS[label] + raise Failure, "SPEC roster has a section for unknown runner #{label.inspect}" if runner.nil? + + seen << runner + roster[runner] + next + end + + # FAIL-CLOSED on anything list-shaped, rather than recognising one spelling + # and skipping the rest. `next unless line.start_with?("- ")` silently + # dropped every other valid Markdown list form — indented, `*`, `+`, `1.` — + # and a STALE entry written that way was then absent from the roster set, so + # no mismatch arose and the gate passed. A false green, which is the one + # outcome this extractor may not produce. + # + # The answer is not a third selector per spelling. It is to invert the + # default: a line that looks like a list item in ANY form must be the + # canonical `- "case name"`, or it is an error. One predicate closes the + # whole class, including spellings nobody has written yet. Prose + # continuation lines (the roster's headings wrap, and Python's section is a + # sentence) are untouched because they are not list-shaped. + next unless line.match?(/\A\s*([-*+]|\d+[.)])\s/) + + raise Failure, "SPEC roster bullet before any runner heading: #{line.strip[0, 60]}" if runner.nil? + + name = line[/\A-\s+"([^"]+)"/, 1] + if name.nil? + raise Failure, "SPEC roster line is list-shaped but not a canonical bullet " \ + "(`- \"case name\" — reason`): #{line.strip[0, 80]}. Written another way it " \ + "would be skipped, and a stale entry that is skipped never contradicts " \ + "anything." + end + + roster[runner] << name + end + + missing = EXPECTED_RUNNERS - seen + unless missing.empty? + raise Failure, "SPEC roster has no section for: #{missing.join(', ')} - a runner without a " \ + "heading contributes nothing and its skips go unrecorded" + end + + # A duplicated heading splits one runner's lines across two sections, so + # neither reads as the whole set and "one line per runner x test" is already + # broken before any comparison. + repeated = seen.tally.select { |_, n| n > 1 }.keys + unless repeated.empty? + raise Failure, "SPEC roster has more than one section for: #{repeated.sort.join(', ')}" + end + + # A case listed twice under one runner is invisible to the comparison: + # Array#- removes EVERY matching occurrence, so `actual - stated` and + # `stated - actual` are both empty and the duplicate passes — with two + # possibly conflicting classifications attached to one case. Codex found it, + # and it is the same silent-pass shape as the duplicate block above. + roster.each do |run_id, names| + dupes = names.tally.select { |_, n| n > 1 }.keys + next if dupes.empty? + + raise Failure, "SPEC roster lists #{dupes.first.inspect} twice under #{run_id}; the section " \ + "promises one line per runner x test, and a repeat is invisible to the " \ + "set comparison" + end + + roster +end + +# Compares the roster's enumeration against what the runners reported. +def check_roster(manifests, spec_path) + roster = parse_roster(spec_path) + errors = [] + + manifests.each do |m| + actual = m.excluded.keys.map(&:last) + + # The roster identifies a case by NAME alone, so it cannot express two + # same-named cases from different fixtures. No runner excludes such a pair + # today; if one ever does, the roster needs file qualifiers and this says so + # rather than silently comparing an ambiguous set. + dupes = actual.tally.select { |_, n| n > 1 }.keys + unless dupes.empty? + errors << "#{m.runner} excludes #{dupes.first.inspect} in more than one fixture; the SPEC " \ + "roster identifies cases by name alone and cannot express that. Add the fixture " \ + "to those roster lines and teach this check to read it." + next + end + + stated = roster[m.runner] + (actual - stated).each do |name| + errors << "#{m.runner} excludes #{name.inspect} and the SPEC roster does not list it" + end + (stated - actual).each do |name| + errors << "the SPEC roster lists #{name.inspect} for #{m.runner}, which no longer excludes it" + end + end + + errors +end + def run(partial:) manifests = load_all present = manifests.map(&:runner).sort @@ -187,6 +358,26 @@ def run(partial:) "cannot produce all six.)" end + # BEFORE the partial branch, deliberately. Roster drift is checkable against + # whatever manifests exist: "does the roster's line for Ruby match what Ruby + # reported" needs Ruby's manifest and nothing else. Placing it after the + # partial return meant the normal Linux `make check` path — which always + # passes --partial, because Swift's manifest is macOS-only — never checked the + # roster at all, so a stale Go or Ruby line passed locally and only the CI + # fan-in could catch it. Both bots found that. + # + # Partial input relaxes exactly one thing, the all-six overlap verdict below, + # because that is the only claim needing every runner. A runner that did not + # report simply is not compared; its roster section is neither confirmed nor + # contradicted. + roster_errors = check_roster(manifests, File.join(ROOT, "SPEC.md")) + unless roster_errors.empty? + roster_errors.each { |e| warn "FAIL: #{e}" } + warn " SPEC section 19's Zero-Skip roster claims to enumerate every skip verbatim from " \ + "the runners' skip mechanisms (#736). It is restated by hand, so it drifts." + return 1 + end + # name => the runners that did NOT execute it by_case = Hash.new { |h, k| h[k] = [] } manifests.each do |m| diff --git a/scripts/test-check-fixture-execution.rb b/scripts/test-check-fixture-execution.rb index 6a7298bf9b..6939ffc8cb 100755 --- a/scripts/test-check-fixture-execution.rb +++ b/scripts/test-check-fixture-execution.rb @@ -22,6 +22,8 @@ GATE = File.join(__dir__, "check-fixture-execution.rb") RUNNERS = %w[go kotlin python ruby swift typescript].freeze +ROSTER_LABELS = { "go" => "Go", "kotlin" => "Kotlin", "python" => "Python", + "ruby" => "Ruby", "swift" => "Swift", "typescript" => "TypeScript" }.freeze failures = [] @@ -34,14 +36,35 @@ def default_manifests end end +# A SPEC roster that agrees with whatever the manifests say, so roster drift is +# only ever exercised by a case that asks for it. +def default_spec(manifests) + labels = ROSTER_LABELS + body = RUNNERS.map do |runner| + lines = ["**#{labels.fetch(runner)}** (`x`):"] + # Defensive: some cases replace a manifest with a non-Hash or nil on + # purpose, and this helper only exists to keep the roster in step with the + # ones that are real. + entry = manifests[runner] + excluded = entry.is_a?(Hash) ? (entry["excluded"] || []) : [] + excluded.each do |e| + lines << %(- "#{e['name']}" — because.) + end + lines.join("\n") + end.join("\n\n") + + "# Spec\n\n\n#{body}\n\n" +end + # Materialise the manifests (after `mutate` has had a chance to break one) and # run the gate against them. -def gate(mutate = nil, partial: false) +def gate(mutate = nil, partial: false, spec: nil) manifests = default_manifests mutate&.call(manifests) Dir.mktmpdir do |dir| FileUtils.mkdir_p(File.join(dir, "conformance", "manifests")) + File.write(File.join(dir, "SPEC.md"), spec || default_spec(manifests)) manifests.each do |runner, body| next if body.nil? # a nil entry means "this runner did not report" @@ -215,6 +238,112 @@ def exclude(manifests, name, runners, file: "alpha.json") } expect_fail(failures, "one case excluded twice in one manifest", out, status, "is excluded twice") +# --- SPEC roster set-equality (#736) ----------------------------------------- + +# A runner skip the roster does not list. This is the drift that was ALREADY in +# SPEC when the check was written: Kotlin and Swift excluded the link-header +# case via their tag branch while the roster described it in prose. +out, status = gate(lambda { |m| exclude(m, "unlisted skip", ["ruby"]) }, + spec: "\n" + + RUNNERS.map { |r| "**#{ ROSTER_LABELS.fetch(r) }** (`x`):" }.join("\n\n") + + "\n\n") +expect_fail(failures, "a runner skip missing from the roster", out, status, + "the SPEC roster does not list it") + +# The opposite drift: a roster line for a skip that has been closed. "A PR that +# closes a gap deletes exactly its own lines" is the roster's own rule, and it +# was enforced by nothing. +out, status = gate(nil, + spec: "\n" + + RUNNERS.map { |r| + head = "**#{ ROSTER_LABELS.fetch(r) }** (`x`):" + r == "go" ? "#{head}\n- \"a closed gap\" — stale." : head + }.join("\n\n") + + "\n\n") +expect_fail(failures, "a roster line for a skip that no longer exists", out, status, + "which no longer excludes it") + +# A runner with no heading contributes nothing, so its skips would go unchecked. +out, status = gate(nil, spec: "\n**Go** (`x`):\n\n") +expect_fail(failures, "a runner with no roster section", out, status, "has no section for") + +# No roster at all must not read as "the roster agrees". +out, status = gate(nil, spec: "# Spec\n\nNo roster here.\n") +expect_fail(failures, "SPEC with no roster block", out, status, "holds no") + +# A bullet the extractor cannot read is a name missing from the roster set — +# loud, never a silent pass. This is the property that makes the parser +# acceptable at all. +out, status = gate(nil, + spec: "\n**Go** (`x`):\n- unquoted case name\n" + + (RUNNERS - ["go"]).map { |r| "**#{ ROSTER_LABELS.fetch(r) }** (`x`):" }.join("\n\n") + + "\n\n") +expect_fail(failures, "a roster bullet without a quoted name", out, status, + "list-shaped but not a canonical bullet") + +# Roster drift must be caught in PARTIAL mode too. The normal Linux `make +# check` path always passes --partial (Swift's manifest is macOS-only), so a +# roster check that ran only in full mode never ran locally at all — a stale Go +# or Ruby line would reach CI untouched. Partial input relaxes the all-six +# overlap verdict and nothing else. +out, status = gate(lambda { |m| + exclude(m, "unlisted skip", ["ruby"]) + m["swift"] = nil +}, partial: true, + spec: "\n" + + RUNNERS.map { |r| "**#{ROSTER_LABELS.fetch(r)}** (`x`):" }.join("\n\n") + + "\n\n") +expect_fail(failures, "roster drift is caught in partial mode", out, status, + "the SPEC roster does not list it") + +# A SECOND complete roster block is not compared, so a stale line inside it +# would pass unnoticed — a silent pass, the one failure mode this extractor is +# not allowed to have. +one_block = RUNNERS.map { |r| "**#{ROSTER_LABELS.fetch(r)}** (`x`):" }.join("\n\n") +out, status = gate(nil, + spec: "\n#{one_block}\n\n" \ + "\n\n#{one_block}\n\n") +expect_fail(failures, "two roster blocks in SPEC", out, status, "exactly one") + +# One case listed twice under a runner. Array#- removes every matching +# occurrence, so both diffs come back empty and the duplicate passes — carrying +# two possibly conflicting classifications for one case. +out, status = gate(lambda { |m| exclude(m, "listed twice", ["go"]) }, + spec: "\n" + + RUNNERS.map { |r| + head = "**#{ROSTER_LABELS.fetch(r)}** (`x`):" + r == "go" ? "#{head}\n- \"listed twice\" — a.\n- \"listed twice\" — b." : head + }.join("\n\n") + + "\n\n") +expect_fail(failures, "one case listed twice under a runner", out, status, "twice under go") + +# Two sections for one runner splits its lines, so neither reads as the whole +# set and the contract is broken before any comparison runs. +out, status = gate(nil, + spec: "\n" + + (RUNNERS.map { |r| "**#{ROSTER_LABELS.fetch(r)}** (`x`):" } + + ["**Go** (`x`):"]).join("\n\n") + + "\n\n") +expect_fail(failures, "two roster sections for one runner", out, status, "more than one section") + +# A STALE entry written with any non-canonical list marker. `start_with?("- ")` +# skipped these, so the entry never entered the roster set, never contradicted a +# manifest, and the gate passed — a false green, the one outcome this extractor +# may not produce. Fixed by inverting the default: list-shaped means canonical +# or error, which closes every spelling at once rather than one at a time. +[" - \"indented stale\" — x.", "* \"asterisk stale\" — x.", + "+ \"plus stale\" — x.", "1. \"ordered stale\" — x."].each do |bullet| + out, status = gate(nil, + spec: "\n" + + RUNNERS.map { |r| + head = "**#{ROSTER_LABELS.fetch(r)}** (`x`):" + r == "go" ? "#{head}\n#{bullet}" : head + }.join("\n\n") + + "\n\n") + expect_fail(failures, "stale roster entry as #{bullet.strip[0, 12]}", out, status, + "list-shaped but not a canonical bullet") +end + # --- report ------------------------------------------------------------------ if failures.empty?