Skip to content

fix(cli): kates test apply refuses a scenario spec number key that holds something other than a number - #381

Merged
bmscomp merged 1 commit into
mainfrom
fix/audit-12-spec-numbers
Oct 18, 2026
Merged

bmscomp merged 1 commit into
mainfrom
fix/audit-12-spec-numbers

Conversation

@bmscomp

@bmscomp bmscomp commented Oct 17, 2026

Copy link
Copy Markdown
Owner

Why

kates test apply sends 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 are 10m or 5M. yaml.v3 decodes these values as strings. Then test apply --wait runs 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 with toInt or toIntPtr. Both turn a string, or a bare key's nil, into "not set" (toInt gives 0, toIntPtr nil), so the key leaves the request.
  • scenarioSpecProblems (:515-523), which test apply runs before any test starts (:150-158), checks only the three enable keys.
  • The validate block is typed float64 (ValidationSpec, :29-39), so the same quoted value there is an error when the file is read. Only the spec block is silent.
  • The finding said scenario-diff never 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 only test apply submits the run without a word.
  • The MCP draft_scenario tool 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 once apply refuses the value.

The plan also found three places the fix as written missed:

  • scenario-diff calls scenarioSpecProblems too (scenario_diff.go:63-66). Once that function checks number keys, scenario-diff refuses the file. So TestScenarioDiff_KeysApplyDoesNotSendAreNotCompared, which has records: "50000" as a Not compared row, would fail.
  • With %v, the message would print a quoted number without its quotes (spec.records is 5000000; write a number). mcpScnShow keeps them.
  • Code inserted at apply.go:510-523 moves validateSLAs and unevaluableSLAs, and the scenario-validate-grading caveat's anchored ref cites both. Adding the slices or maps import to apply.go would move every ref into that file, so the key list is a plain slice.

Changes

cli/cmd/apply.go

  • New scenarioIntKeys, right after scenarioBoolKeys. It lists the 13 number keys in scenarioToRequest's order: records, parallelProducers, recordSizeBytes, durationSeconds, batchSize, lingerMs, numConsumers, replicationFactor, partitions, minInsyncReplicas, targetThroughput, fetchMinBytes and fetchMaxWaitMs.
  • scenarioSpecProblems adds spec.<key> is <value>; write a number for each of those keys that is present and that toIntPtr cannot read. That covers a quoted number, a unit and a bare key (is empty). It checks the number keys before the enable keys, following scenarioToRequest's order.
  • test apply already reports these problems before any test starts. The error reads: scenario 2 (second): spec.records is "5000000"; write a number. scenario-diff refuses the file the same way, instead of listing the key under Not compared.
  • No import added, and nothing above line 510 moved.

cli/cmd/mcp_tools_scenario.go

  • mcpScnCheckSpecNumber: a value that is not a number is now invalid, 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. The sendsZero wording branch is gone, and the doc comment says what the function now flags.
  • The scenario-validate-grading caveat's ref cli/cmd/apply.go:…,525-580,597-622 becomes …,538-593,610-635. It still points at the moved validateSLAs and unevaluableSLAs, and the same anchors hold it there. 29-39, 217-250 and 339-351 stay.

Docs

  • Book 13's spec reference: one sentence at the end of the enable keys paragraph (:94). The number keys take a number. kates test apply and kates scenario-diff refuse 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: 10m or a bare number key used to run on the type defaults without a word. Now kates test apply exits 1 before it starts any test. That is the point of the fix, and it is worth a release-note line. No file under cli/examples, examples or docs has 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: 10m and a bare lingerMs: give 3 problems, among them spec.records is "5000000"; write a number and spec.lingerMs is empty; write a number.
    • records: 5000000, lingerMs: 0 and durationSeconds: 1.5 give none.
  • TestApply_RefusesANumberThatIsNotANumber (apply_test.go), modelled on TestApply_RefusesAFlagThatIsNotABoolean. It uses a two-scenario file whose second scenario has records: "5000000". The error names scenario 2 (second): spec.records, and 0 requests reach the httptest server.
  • TestScenarioDiff_RefusesANumberApplyRefuses (cli/cmd/scenario_diff_test.go): scenario-diff refuses a scenario with records: "50000" and names scenario 1 (quoted): spec.records.
  • TestScenarioDiff_KeysApplyDoesNotSendAreNotCompared: the quoted records input and its Not compared row are gone, because scenario-diff now refuses that file.
  • cli/cmd/mcp_tools_scenario_test.go:458: spec.records is now invalid, with kates test apply refuses it. The draft's verdict there was already invalid, because of acks: "2".
  • TestMCPDraftScenarioSpecKeysMatchScenarioToRequest: scenarioIntKeys must equal the i-kind keys of mcpScnSpecKeys, and scenarioBoolKeys the b-kind keys. That test already holds mcpScnSpecKeys to the keys apply.go reads, so a number key added to scenarioToRequest cannot skip the check.
  • TestMCPCaveatRefAnchors passes with the re-pointed ref.

Run locally, all passing:

  • git diff --name-only origin/main...HEAD: 1 commit.
    • Changed: cli/cmd/{apply.go,apply_test.go,mcp_tools_scenario.go,mcp_tools_scenario_test.go,scenario_diff_test.go} and docs/book/13-scenario-files.md.
  • cd cli && go test ./...: every package passed. cmd took 32 s, and it includes the caveat-ref anchor tests.
  • cd cli && go test -p 1 ./... -race -count=1: every package passed. This is the ci.yml go-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.sh and scripts/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 --check and scripts/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 of docs/ and scripts/: 39 chapters prepared, 38 pages rendered.
  • python3 scripts/check-book-fragments.py on 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.yml Markdown Link Check): lychee is not installed locally. The new book sentence has no link, so this job cannot gain a failure from this PR.
  • Helm, Makefile, actionlint, shellcheck and yamllint checks: not triggered, because no file under charts/**, .github/** or scripts/**, no Makefile and no *.yml/*.yaml file changed.

Run only in CI:

  • The Java backend suite (ci.yml Build & Test, ./mvnw verify) and the Java *IT tests. kates/ is unchanged, so the result is the same as on main.
  • docs.yml Markdown Link Check (lychee offline), which is not installed locally.
  • The book.yml Render PDF job (TinyTeX and EB Garamond, with the dropped-character and overfull checks).
  • The native image (native.yml) is not triggered, because kates/ is unchanged.
  • The Kind integration tests (integration.yml) and the Docker image builds (ci-docker.yml).
  • The github-advanced-security AI scan, which is red on every PR because of a GitHub-side error.

Decisions

  • An explicit scenarioIntKeys slice, with a sync test. The other option was to walk mcpScnSpecKeys for kind i. That map has no order, so the problems would need sorting, and the slices/maps imports would move every cited apply.go line. The test against mcpScnSpecKeys stops the slice from drifting.
  • A bare key is refused as is empty; write a number. The enable keys already refuse a bare key this way (toBoolPtr's comment, apply.go:492-496). A bare records: 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-diff no 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

  • No edit to book 13's troubleshooting item 3 (:455-470), which explains that a quoted number in the validate block is an error. The plan marked it optional.
  • toIntPtr's doc comment is unchanged. It still says a value that is not a number "sends nothing", and does not say that scenarioSpecProblems now refuses it first. The comment sits above line 510. Editing it would move the anchored refs into apply.go, and the plan said to edit nothing above 510.
  • The scenario-diff refusal is its own test function, not a second case in TestScenarioDiff_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:

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:

  1. Rebase onto main.
  2. Build a diff line map of each file that moved, from this branch's base to the merged file.
  3. Re-point the moved caveat refs from that map.
  4. Run cd cli && go test ./cmd -run TestMCPCaveat.

…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.
@bmscomp
bmscomp merged commit 00b990d into main Oct 18, 2026
58 checks passed
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.

1 participant