Skip to content

fix(cli): report exports read each phase's name, metrics and SLA verdict - #382

Merged
bmscomp merged 1 commit into
mainfrom
fix/audit-13-report-phases
Oct 18, 2026
Merged

bmscomp merged 1 commit into
mainfrom
fix/audit-13-report-phases

Conversation

@bmscomp

@bmscomp bmscomp commented Oct 17, 2026

Copy link
Copy Markdown
Owner

Why

The CLI never decoded a report's phases. Checked in the code:

  • The backend's shape. ReportGenerator builds each phase as a PhaseReport with phaseName, metrics (a ReportSummary) and slaVerdict (ReportGenerator.java:239-249, PhaseReport.java:18-22). docs/book/11-api-reference.md documents the same shape.
  • The CLI's shape. client.ReportPhase read name, status, recordsSent, throughputRecordsPerSec, p99LatencyMs and errorRate (cli/client/types.go:289-296). None of these keys exist, so every field stayed empty.
  • What came out. Every LOAD report has a produce and a consume phase. kates report export --format md wrote each one as | main | ✓ | 0 | 0.0 rec/s | 0.00 | 0.0000% |: the empty name became main, and the empty status got the pass mark. --format html did the same with a badge-pass. A phase whose SLA failed was marked as passed too. These exports go into CI artifacts and PRs.
  • Why the tests passed. The renderer tests built ReportPhase by hand in the Go shape, and TestReport served the Go struct encoded as JSON, so the client agreed with itself.

The finding missed two things:

  • Escaping. Once phaseName decodes, it holds the phase names from the user's scenario file. fix(engine): refuse scenario values the database cannot store #329 limits only their length and NUL bytes. report_html.go wrote the name into the HTML unescaped, which would be an HTML injection in a file people share. In Markdown, a | in the name would split the row.
  • The report show fallback. When the summary is missing or its throughput is 0, report show summed the phases to fill it in. Phases are built from the same rows as the summary, so their sum is 0 too. The branch never did anything, and it could only read the broken fields.

Changes

cli/client/types.go

  • ReportPhase has the backend's shape: Name (phaseName), Metrics *ReportSummary and SlaVerdict *SlaVerdict, with a comment naming PhaseReport.

cli/cmd/report_markdown.go, cli/cmd/report_html.go

  • Records, throughput, P99 and errors come from the phase's metrics. A phase with no metrics falls back to a zero ReportSummary.
  • The column is renamed from Status to SLA. It shows ✓ or ✖ from the phase's slaVerdict, and — when there is none. In HTML the cell is a ✓ PASSED or ✖ FAILED badge, the same text as the overall SLA Verdict card.
  • The phase name is written as given, with no main stand-in. HTML escapes it with html.EscapeString, and Markdown escapes | the way the violations table already does.

cli/cmd/report.go

  • report show no longer sums phases. Only the phase branch of the fallback is gone. The GetTest fallback, which sums the run's results, is unchanged apart from its indent.

Visible changes

  • kates report show -o json re-encodes client.Report, so each phase now prints phaseName, metrics and slaVerdict. The old keys (name, status, recordsSent, throughputRecordsPerSec, p99LatencyMs, errorRate) are gone. They were always empty or 0. Book 10 points users at this command for the full report as JSON.
  • Markdown and HTML exports in CI artifacts start showing ✖ for phases whose SLA failed, where they always showed ✓ before.
  • A report whose summary has 0 throughput and has phases now makes one more GET /api/tests/{id}, through the GetTest fallback. It reads the same rows, so the outcome is the same.

Tests

  • TestReport (cli/client/client_test.go) serves a raw JSON literal in the backend's shape: a summary and two phases, each with phaseName, metrics and slaVerdict, the second one failed. It asserts two phases decode, the produce phase's throughput and P99, and the consume phase's name and failed verdict. The old struct decodes this JSON to two phases with no name and no numbers.
  • TestRenderMarkdownReport_WithPhases asserts:
    • the produce row carries its own records, throughput and P99: | produce | ✓ | 600 | 10000.0 rec/s | 15.50 | 0.0000% |;
    • a phase whose SLA failed shows ✖;
    • a phase named a|b renders as a\|b, and a phase with no verdict shows —;
    • no | main | row appears.
  • TestRenderHTMLReport_WithPhases asserts a passing phase gets badge-pass, a failing one gets badge-fail, and a phase named <script> renders as &lt;script&gt; with no raw <script> in the output.
  • Run locally, all passing:
    • git diff --name-only origin/main...HEAD: 7 files, all cli/** Go (client/types.go, client_test.go, cmd/report*.go and their tests). They trigger ci.yml (code filter), lint.yml (go), CodeQL (go), integration.yml and ci-docker.yml.
    • cd cli && go test -p 1 -race -count=1 ./..., CI's flags: all packages ok (cmd in 147.8 s). The TestMCPCaveat* tests ran inside the cmd package and pass.
    • cd cli && go vet ./...: clean.
    • cd cli && golangci-lint run --new-from-rev=origin/main ./...: 0 issues, with golangci-lint 2.14.0, which matches the GOLANGCI_LINT_VERSION pin.
    • cd cli && govulncheck ./...: no vulnerabilities found, with govulncheck v1.7.0, which matches the pin.
    • scripts/check-cli-style.sh, with GOMEMLIMIT=2GiB GOGC=50 as in CI.
    • scripts/check-cli-compat.sh: all terminal-compatibility checks ok.
    • helm lint charts/kates/, helm template kates charts/kates/ and scripts/check-versions.sh. The ci.yml Helm Lint job runs on the code filter, so it runs here too. The charts are unchanged; its Strimzi operator lint steps were not run.
  • The backend is unchanged: no kates/ file is touched. The full backend suite ran on main and passed, so it gives the same result here.
  • Not run locally, because no path they watch changed: the book, the charts set, the Makefile, actionlint, shellcheck and the docs checks.
  • Only in CI:
    • integration.yml Kind Cluster Smoke Tests (cli/** trigger);
    • ci-docker.yml Docker image build validation (cli/** trigger);
    • codeql.yml Go CodeQL analysis;
    • ci.yml Build & Test, the Java suite with the *IT tests (kates/ unchanged);
    • native.yml native image;
    • the github-advanced-security AI scan, which is always red (GitHub-side).

Decisions

  • The column is SLA, not Status. The cell shows the phase's own SLA verdict. A phase has no status of its own: ReportGenerator judges phases on their SLA alone, and the status belongs to the run.
  • — when a phase has no slaVerdict. The current backend always sets one, so this only matters for odd input, and it doesn't claim a pass nothing judged.
  • Only the phase branch of report.go's fallback is deleted. The GetTest branch is just as dead against the current backend, which always sends a summary built from the same rows, but it still matters for an older Kates API that sent none.
  • cli/cmd/data/doc_entries.yaml:817-818 is left for a follow-up.

Deliberately not done

  • cli/cmd/data/doc_entries.yaml:817-818 still says report show displays per-phase breakdowns. report show renders the summary and the SLA verdict, not the phases. This is left for a follow-up, as decided.
  • ReportPhase has no phaseType or timeSeries. ReportGenerator never sets phaseType, and nothing in the CLI renders timeSeries.
  • A run still in flight gets a passing verdict for each phase from the backend, so its phases show ✓, the same as its overall verdict.

Landing

Shares no file with the earlier plan PRs, so it can land at any time.

The backend writes a report phase as {phaseName, metrics, slaVerdict}, but
the CLI read name, status, recordsSent, throughputRecordsPerSec,
p99LatencyMs and errorRate, none of which exist. Every phase of a LOAD
report, produce and consume alike, came out of `report export --format md`
and `--format html` as "main ✓ 0 0.0 rec/s", a phase whose SLA failed
included. The client test encoded the Go struct, so it agreed with itself.

ReportPhase now has the backend's shape. Both renderers take the numbers
from the phase's metrics and mark the column, renamed from Status to SLA,
✓ or ✖ from its slaVerdict (— when there is none). The phase name is the
user's scenario text, so the HTML export escapes it and the Markdown one
escapes |. report show no longer sums phases to fill in a summary of 0:
they are built from the same rows as the summary, so the sum is 0 too.
`report show -o json` now prints phaseName, metrics and slaVerdict in
place of the old keys, which were always empty. TestReport serves the
backend's JSON as written.
@bmscomp
bmscomp merged commit f528f1d into main Oct 18, 2026
55 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