Repository navigation
fix(cli): kates test apply refuses a scenario spec number key that holds something other than a number - #381
Merged
Merged
Conversation
…lds something other than a number A scenario with spec records: "5000000" and durationSeconds: 10m, common when CI templates the file, decodes both as strings. scenarioToRequest read them as no number and left them out of the request, so the run used the type's default records and duration, and test apply --wait graded the validate gates against that much shorter run and exited 0. Only the validate block, typed float64, made such a value an error. scenarioSpecProblems now checks the 13 number keys scenarioToRequest reads, listed in scenarioIntKeys, as it already checked the three enable keys. A quoted number, a unit or a bare key is refused before any test starts, as spec.records is "5000000"; write a number, and scenario-diff refuses the file the same way instead of listing the key under Not compared. draft_scenario reports such a key as invalid, with the wording of its true/false check, where it used to warn that the value was left out of the request. A test holds scenarioIntKeys and scenarioBoolKeys to the key kinds draft_scenario describes, so a new key cannot skip the check. The scenario-validate-grading caveat's apply.go ref is re-pointed to the moved validateSLAs and unevaluableSLAs, and the book's spec reference says the number keys take a number.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
kates test applysends a scenario spec number key that holds something other than a number as nothing at all. It prints no error, and the run uses the type's default.Take a scenario with
spec: {records: "5000000", durationSeconds: 10m}. A quoted number is common when CI templates the file (records: "${RECORDS}"), and so are10mor5M. yaml.v3 decodes these values as strings. Thentest apply --waitruns the type's default records and duration, grades the validate gates against that much shorter run, and exits 0.Checked in the code on main:
scenarioToRequest(cli/cmd/apply.go:372-445) reads 13 number keys withtoIntortoIntPtr. Both turn a string, or a bare key's nil, into "not set" (toIntgives 0,toIntPtrnil), so the key leaves the request.scenarioSpecProblems(:515-523), whichtest applyruns before any test starts (:150-158), checks only the threeenablekeys.float64(ValidationSpec, :29-39), so the same quoted value there is an error when the file is read. Only the spec block is silent.scenario-diffnever checks this either. It does: it lists such a key under Not compared, with "it is not a number, so kates test apply leaves it out" (scenario_diff.go:334-346). So onlytest applysubmits the run without a word.draft_scenariotool already flags the case (mcp_tools_scenario.go:918-925), but as a warning that the value "is left out of the request". That would stop being true onceapplyrefuses the value.The plan also found three places the fix as written missed:
scenario-diffcallsscenarioSpecProblemstoo (scenario_diff.go:63-66). Once that function checks number keys,scenario-diffrefuses the file. SoTestScenarioDiff_KeysApplyDoesNotSendAreNotCompared, which hasrecords: "50000"as a Not compared row, would fail.%v, the message would print a quoted number without its quotes (spec.records is 5000000; write a number).mcpScnShowkeeps them.apply.go:510-523movesvalidateSLAsandunevaluableSLAs, and thescenario-validate-gradingcaveat's anchored ref cites both. Adding theslicesormapsimport toapply.gowould move every ref into that file, so the key list is a plain slice.Changes
cli/cmd/apply.goscenarioIntKeys, right afterscenarioBoolKeys. It lists the 13 number keys inscenarioToRequest's order: records, parallelProducers, recordSizeBytes, durationSeconds, batchSize, lingerMs, numConsumers, replicationFactor, partitions, minInsyncReplicas, targetThroughput, fetchMinBytes and fetchMaxWaitMs.scenarioSpecProblemsaddsspec.<key> is <value>; write a numberfor each of those keys that is present and thattoIntPtrcannot read. That covers a quoted number, a unit and a bare key (is empty). It checks the number keys before theenablekeys, followingscenarioToRequest's order.test applyalready reports these problems before any test starts. The error reads:scenario 2 (second): spec.records is "5000000"; write a number.scenario-diffrefuses the file the same way, instead of listing the key under Not compared.cli/cmd/mcp_tools_scenario.gomcpScnCheckSpecNumber: a value that is not a number is nowinvalid, with the wording of the true/false check:it is "5000000"; kates test apply refuses it: write a number. It used to be a warning that the value is left out of the request. ThesendsZerowording branch is gone, and the doc comment says what the function now flags.scenario-validate-gradingcaveat's refcli/cmd/apply.go:…,525-580,597-622becomes…,538-593,610-635. It still points at the movedvalidateSLAsandunevaluableSLAs, and the same anchors hold it there.29-39,217-250and339-351stay.Docs
enablekeys paragraph (:94). The number keys take a number.kates test applyandkates scenario-diffrefuse a file where one holds a quoted number, a unit or nothing, naming the scenario and the key, before any test starts.Behaviour change. A scenario with
records: "${RECORDS}",durationSeconds: 10mor a bare number key used to run on the type defaults without a word. Nowkates test applyexits 1 before it starts any test. That is the point of the fix, and it is worth a release-note line. No file undercli/examples,examplesordocshas a quoted number.draft_scenario's verdict for such a draft changes from warning to invalid. No golden or eval fixture asserts the old text.Tests
Added or changed:
TestScenarioSpecProblems_ANumberKeyThatIsNotANumber(cli/cmd/apply_test.go):records: "5000000",durationSeconds: 10mand a barelingerMs:give 3 problems, among themspec.records is "5000000"; write a numberandspec.lingerMs is empty; write a number.records: 5000000,lingerMs: 0anddurationSeconds: 1.5give none.TestApply_RefusesANumberThatIsNotANumber(apply_test.go), modelled onTestApply_RefusesAFlagThatIsNotABoolean. It uses a two-scenario file whose second scenario hasrecords: "5000000". The error namesscenario 2 (second): spec.records, and 0 requests reach the httptest server.TestScenarioDiff_RefusesANumberApplyRefuses(cli/cmd/scenario_diff_test.go):scenario-diffrefuses a scenario withrecords: "50000"and namesscenario 1 (quoted): spec.records.TestScenarioDiff_KeysApplyDoesNotSendAreNotCompared: the quotedrecordsinput and its Not compared row are gone, becausescenario-diffnow refuses that file.cli/cmd/mcp_tools_scenario_test.go:458:spec.recordsis nowinvalid, withkates test apply refuses it. The draft's verdict there was already invalid, because ofacks: "2".TestMCPDraftScenarioSpecKeysMatchScenarioToRequest:scenarioIntKeysmust equal thei-kind keys ofmcpScnSpecKeys, andscenarioBoolKeystheb-kind keys. That test already holdsmcpScnSpecKeysto the keysapply.goreads, so a number key added toscenarioToRequestcannot skip the check.TestMCPCaveatRefAnchorspasses with the re-pointed ref.Run locally, all passing:
git diff --name-only origin/main...HEAD: 1 commit.cli/cmd/{apply.go,apply_test.go,mcp_tools_scenario.go,mcp_tools_scenario_test.go,scenario_diff_test.go}anddocs/book/13-scenario-files.md.cd cli && go test ./...: every package passed.cmdtook 32 s, and it includes the caveat-ref anchor tests.cd cli && go test -p 1 ./... -race -count=1: every package passed. This is theci.ymlgo-tests invocation, with the race detector and no cache.cd cli && go vet ./...: no findings.cd cli && golangci-lint run --new-from-rev=origin/main ./...: 0 issues.cd cli && govulncheck ./...: no vulnerabilities found.scripts/check-cli-style.shandscripts/check-cli-compat.sh: OK.scripts/check-book-style.sh: style checks and the structure ratchet are OK.13-scenario-files.md's changelog phrases went from 1 to 0, an improvement within the baseline.scripts/gen-version-matrix.sh --checkandscripts/gen-chart-table.sh --check: in sync.python3 scripts/check-book-contrast.py: all 170 colour pairs meet WCAG AA.scripts/check-mermaid.sh --quiet: 156 diagrams, 0 invalid.scripts/prepare-book-render.sh && quarto render docs/book --to html, on a scratch copy ofdocs/andscripts/: 39 chapters prepared, 38 pages rendered.python3 scripts/check-book-fragments.pyon that render: all 7,458 fragment links in 38 pages land on an id.The backend is unchanged. The full backend suite ran on main and passed, and gives the same result here, so it was not run again on this branch.
Not run locally:
lychee --offline --include-fragments(docs.ymlMarkdown Link Check): lychee is not installed locally. The new book sentence has no link, so this job cannot gain a failure from this PR.Makefile,actionlint,shellcheckandyamllintchecks: not triggered, because no file undercharts/**,.github/**orscripts/**, noMakefileand no*.yml/*.yamlfile changed.Run only in CI:
ci.ymlBuild & Test,./mvnw verify) and the Java*ITtests.kates/is unchanged, so the result is the same as on main.docs.ymlMarkdown Link Check (lychee offline), which is not installed locally.book.ymlRender PDF job (TinyTeX and EB Garamond, with the dropped-character and overfull checks).native.yml) is not triggered, becausekates/is unchanged.integration.yml) and the Docker image builds (ci-docker.yml).github-advanced-securityAI scan, which is red on every PR because of a GitHub-side error.Decisions
scenarioIntKeysslice, with a sync test. The other option was to walkmcpScnSpecKeysfor kindi. That map has no order, so the problems would need sorting, and theslices/mapsimports would move every citedapply.goline. The test againstmcpScnSpecKeysstops the slice from drifting.is empty; write a number. Theenablekeys already refuse a bare key this way (toBoolPtr's comment,apply.go:492-496). A barerecords:used to run the type default without a word. Now the file must give a number or leave the key out.scenarioDiffNotSent's "it is not a number" fallback stays (scenario_diff.go:336-346).scenario-diffno longer reaches it for a number key, because the refusal comes first. It is harmless, and deleting it is a second concern.Deliberately not done
toIntPtr's doc comment is unchanged. It still says a value that is not a number "sends nothing", and does not say thatscenarioSpecProblemsnow refuses it first. The comment sits above line 510. Editing it would move the anchored refs intoapply.go, and the plan said to edit nothing above 510.scenario-diffrefusal is its own test function, not a second case inTestScenarioDiff_RefusesAFlagApplyRefuses. The plan allowed either.Landing
Lands after #375 (plan PR 6), #376 (plan PR 7) and #377 (plan PR 8), because they edit files this PR also edits:
cli/cmd/mcp_tools_scenario.go: all three re-point Java refs in the caveat list. fix(engine): a status poll writes only over the status it read, so a cancelled run stays cancelled #377 edits main's :1247 and :1263, fix(engine): each INTEGRITY run reads in a group of its own, and a consumer with no committed offset starts at its task's first record, not the topic's oldest #376 :1285 and fix(chaos): compound runs and resilience faults go through the safety guard and the disruption lease #375 :1289. This PR edits :1279 (theapply.goref), and :901-924 inmcpScnCheckSpecNumber. No two edits share or touch a line, so the merge should need no hand edits.docs/book/13-scenario-files.md: fix(engine): each INTEGRITY run reads in a group of its own, and a consumer with no committed offset starts at its task's first record, not the topic's oldest #376 edits theconsumerGrouprow at :120, and this PR edits :94.None of the three edits
apply.go, and this PR edits no Java file, so no ref should need re-pointing. Recipe once those have merged:cd cli && go test ./cmd -run TestMCPCaveat.