From b9546ceb654e108ad478878d076a01e170d3ea1c Mon Sep 17 00:00:00 2001 From: Andreas Trawoeger Date: Sun, 25 Oct 2026 02:29:57 +0100 Subject: [PATCH] fix(check): the verdict reads the child's own report, and only a plan child prints one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bed runner's verdict was derived from a BOOLEAN (the child's exit code) and a report-SHAPED line anywhere in the step's log. Three holes followed from that, and all three were mine to close (opencharly/plugin-check#134): - A log that printed NO report — a build step's output, a deploy trace, any non-plan log — could still credit a PASS, because the reader collected every report-shaped line in the text and counted it without asking whether a report had been printed at all. - A child's own report that says a declared check FAILED did not make the run a FAIL. The report and the exit code come from the same step results, so a child that honours its contract already exits 2; the hole was the case where it does not, and the record then carried `status: FAIL` for a check while the run read PASS. - The report-shaped text was not even required to come from a PLAN runner. The runner knows which child it launched, so it can require that a report comes from a child that renders one. MEASURED: a build whose output ends with a self-consistent report-shaped block banked a PASS for a run whose plan proved NOTHING. The reader now takes the child's OWN report — the LAST report the child printed, because the engine renders the report last — and: - credits NOTHING out of a log that printed no report; - reads the per-check lines of the last report block, not of the whole log; - refuses a block whose step lines disagree with the rollup it printed beside them, because the engine's truncate() bounds a message's length but does not flatten its newlines, so a step's message can carry a report-shaped line; - treats a report whose declared check FAILED as a FAIL of the step, with the failed check named; - reads a report ONLY out of a plan child's log. `planChildReporters` binds each child command the runner launches to the command in this package that renders that child's report, and TestEveryReportRendererIsAPlanChild holds that table against the source in both directions: every command here that renders a report must be in it, and every entry in it must render one. The direction of safety is deliberate: an unreadable report, and a report that did not come from a plan runner, can only WITHHOLD a credit, never manufacture one. A run that proved nothing stays a SKIP. The log the reader parses is the child's MERGED output: the cli seam captures the child's stdout and stderr into one text, so a report cannot be told apart by the stream it was written to — and must not be, because the runner has no other view of the child. The root fix, where the verdict reads the child's step RESULTS instead of a rendering of them, is opencharly/plugin-check#135. The step's own record and the run's headline were already reading the same verdict, so the change is purely additive: `ok:` keeps its one meaning (no failure) and the `status:` key carries the verdict. Closes #134 Assisted-by: Claude Code minimax/minimax-m3.1-flash:cloud (fully tested and validated) --- candy/plugin-check/bed_run.go | 34 ++- candy/plugin-check/bed_status.go | 226 +++++++++++++++++-- candy/plugin-check/bed_status_test.go | 309 ++++++++++++++++++++++++++ 3 files changed, 545 insertions(+), 24 deletions(-) diff --git a/candy/plugin-check/bed_run.go b/candy/plugin-check/bed_run.go index ff38e7c..d594c27 100644 --- a/candy/plugin-check/bed_run.go +++ b/candy/plugin-check/bed_run.go @@ -884,7 +884,13 @@ func runCheckBed(ctx context.Context, ex *sdk.Executor, name string, opts bedRun // The child's own report — the per-check outcomes it printed, and whether it printed a // report at all. Read BEFORE the verdict is decided, because the report is where the // per-check statuses live (opencharly/plugin-check#132). - report := parseCheckReport(logText) + // + // ONLY a plan child's report is read. Every other child in a bed run — a build, a + // deploy, an update, a teardown — prints no report, so report-shaped text in its log + // is CONTENT: reading it would let a build's output bank a PASS for a run whose plan + // proved nothing (opencharly/plugin-check#134; the remaining root fix, where the + // report is not text at all, is opencharly/plugin-check#135). + report, innerSkipped := childReport(argv, logText) // A child that reports a SKIP resolved nothing: it rendered no verdict, so it must not be // banked as a PASS (opencharly/plugin-check#110). Recorded and announced, then the bed // continues — a skip is not a failure, and the reason is what tells a reader that this step @@ -894,22 +900,31 @@ func runCheckBed(ctx context.Context, ex *sdk.Executor, name string, opts bedRun fmt.Fprintf(os.Stderr, "charly check run %s: writing %s: %v\n", name, logPath, writeErr) } res.Step = append(res.Step, stepResult{Name: stepName, Duration: dur, OK: true, Diag: diag, - Skipped: skippedFromRollup(logText), SkipReason: skipReason, + Skipped: innerSkipped, SkipReason: skipReason, Checks: report.Checks}) fmt.Fprintf(os.Stderr, "charly check run %s: [%s] SKIPPED after %s: %s (log: %s)\n", name, stepName, dur.Round(time.Millisecond), skipReason, logPath) return nil } ok := cerr == nil && reply.ExitCode == 0 && diagFail == "" sk := stepResult{Name: stepName, Duration: dur, OK: ok, Diag: diag, - Skipped: skippedFromRollup(logText), Checks: report.Checks} + Skipped: innerSkipped, Checks: report.Checks} // A DECLARED CHECK that did not run is a SKIP, not a pass: the step completed, but it did // not prove what it declared (opencharly/plugin-check#132). The step is still not a // failure — the bed continues and tears down as it would after a pass — but the run's // verdict is no longer a PASS. + // + // A declared check that FAILED is a FAILURE, whatever the exit code says: the thing + // under test is broken, not the harness — the same rule the diagnostic backstop below + // applies to an `error:` line under a zero exit. The report and the exit code are + // derived from the same step results, so a child that honoured its contract already + // exited 2 and this cannot fire for it; it closes the case where a child exits 0 and + // its own report says otherwise (opencharly/plugin-check#134). + reportFail := "" if ok { - if verdict, reason := checkReportVerdict(report); verdict == verdictSkip { - sk.SkipReason = reason - } + reportFail = stepVerdictFromReport(report, &sk) + } + if reportFail != "" { + ok = false } res.Step = append(res.Step, sk) if !ok { @@ -951,6 +966,13 @@ func runCheckBed(ctx context.Context, ex *sdk.Executor, name string, opts bedRun printDiagnosticShapes(os.Stderr, diag) return fmt.Errorf("%s: %s", command, diagFail) } + if reportFail != "" { + // The child was happy; its OWN REPORT was not — it says a declared check failed. + // The thing under test is broken, not the harness: the same class the diagnostic + // backstop above reports as 2 (opencharly/plugin-check#134). + fmt.Fprintf(os.Stderr, "charly check run %s: [%s] FAIL after %s: %s (log: %s)\n", name, stepName, dur.Round(time.Millisecond), reportFail, logPath) + return fmt.Errorf("%s: %s", command, reportFail) + } // A step whose phase asserted NOTHING is not a PASS, and the console says so — the // per-step line is read by whoever watches the run (opencharly/plugin-check#132). if stepStatus(&sk) == verdictSkip { diff --git a/candy/plugin-check/bed_status.go b/candy/plugin-check/bed_status.go index dfe105c..d59f107 100644 --- a/candy/plugin-check/bed_status.go +++ b/candy/plugin-check/bed_status.go @@ -3,6 +3,7 @@ package check import ( "fmt" "regexp" + "strconv" "strings" ) @@ -96,7 +97,12 @@ var checkOutcomeRe = regexp.MustCompile(`^ {2}(PASS|FAIL|SKIP|INFRA) +([A-Za-z][ // skipped" (spec/report's FormatStepResultsText). It is the ONE line that says a plan ran // at all, and it is matched WHOLE (anchored, one line) so a log that merely mentions // "skipped" cannot be mistaken for a report. -var reportRollupRe = regexp.MustCompile(`(?m)^\d+ steps?: \d+ passed, \d+ failed, \d+ skipped$`) +// +// The step COUNT is captured, because the rollup is the report's own statement of how many +// steps it printed — the one self-check the document offers. A report's step lines must +// agree with it, and a block that does not is carrying text the report did not print +// (opencharly/plugin-check#134). +var reportRollupRe = regexp.MustCompile(`(?m)^(\d+) steps?: \d+ passed, \d+ failed, \d+ skipped$`) // isCheckKeyword reports whether a plan step ASSERTED. // @@ -116,23 +122,146 @@ type checkReport struct { // A log without one (a build step's output, a deploy trace) carries no check signal // at all, and the absence is NOT a claim that a check was skipped. Present bool + // Readable is true when the report's step lines agree with the rollup the report + // printed beside them. A block that disagrees is carrying text the report did not + // print, so its per-check lines are not the report's own and NOTHING is credited from + // it (opencharly/plugin-check#134). + Readable bool + // Printed is how many report-shaped lines the block carried; Steps is the count the + // rollup itself printed. They are equal in a readable report, and the difference is + // what an unreadable one says out loud. + Printed int + Steps int // Checks are the child's ASSERTING steps — its declared checks — with the status each - // one resolved to. + // one resolved to. Populated only for a READABLE report. Checks []checkOutcome } -// parseCheckReport reads a child's report out of a step's log text. +// planChildReporters maps the child command a bed run launches to the command in this +// package that renders that child's report. It is the ONE place that says which children +// print a report. +// +// A REPORT is what a PLAN runner prints: the per-check outcomes of a resolved plan. Every +// other child in a bed run — a build, a deploy, an update, a teardown — prints none, so +// report-shaped text in its log is CONTENT, not a verdict. +// +// The runner knows which child it launched (the argv it passed), so the report is read only +// from a plan child's log. Without that rule a container build whose output ends with a +// self-consistent report-shaped block banks a PASS for a run whose plan proved NOTHING — +// the same false PASS this file exists to close (opencharly/plugin-check#134). MEASURED: +// such a block is credited, and the run reads PASS while its plan step proved nothing. +// +// This is the rule that can be stated over a TEXT log today. The root fix — a report that +// is not text at all, so the verdict reads the child's step RESULTS — is opencharly/plugin-check#135. +// +// The table is held against the source by TestEveryReportRendererIsAPlanChild: every +// command in this package that renders a report must be in it, and every entry in it must +// render one. +var planChildReporters = map[string]string{ + "check live": "CheckLiveCmd", + "check box": "CheckBoxCmd", + "check feature run": "CheckFeatureRunCmd", +} + +// planChildReportKey names the entry of planChildReporters a step's child argv names, or "" +// when the child is not a plan runner. // -// It returns the report's presence and its declared checks. Lines that are not report -// lines are ignored: the log also carries the child's own noise (resolution logs, deploy -// traces), and none of that is a check outcome. +// The key is built from the argv the runner launched, and the TABLE decides — the argv +// alone never grants a report. `charly check run` (the bed runner itself) and +// `charly check plan` are `check` verbs that print no report, and they are not in the +// table. +func planChildReportKey(argv []string) string { + if len(argv) < 2 || argv[0] != "check" { + return "" + } + if argv[1] == "feature" { + if len(argv) < 3 { + return "" + } + return "check feature " + argv[2] + } + return "check " + argv[1] +} + +// childReport reads the child's own report out of its log, and the inner-step skip count +// the report carries. +// +// ONLY a plan child's report is read: a build's, a deploy's or a teardown's output is +// CONTENT, and content must never bank a verdict (opencharly/plugin-check#134; the +// remaining root fix, where the report is not text at all, is opencharly/plugin-check#135). +// A non-plan child therefore carries no report and no inner skip count — the same zero it +// carried before, because those logs never printed a report. +// +// The log is the child's MERGED output (the cli seam captures the two streams into one +// text, a whole line at a time), so the report is the LAST block in it: the engine renders +// the report last — live_cmd.go after any forwarded guest output, feature_cmd.go after the +// guest's own output — and the seam reunites the streams in the order they were written. +func childReport(argv []string, logText string) (checkReport, int) { + // The argv names the child; the TABLE says whether that child prints a report. An argv + // that names no entry — a build, a deploy, a teardown, `charly check run` itself — is + // not a plan child, and its output carries no report. + if _, ok := planChildReporters[planChildReportKey(argv)]; !ok { + return checkReport{}, 0 + } + return parseCheckReport(logText), skippedFromRollup(logText) +} + +// parseCheckReport reads a child's OWN report out of a step's log text. +// +// THE CHILD'S OWN REPORT is the LAST report the child printed: the engine renders the +// report LAST (live_cmd.go renders it after any forwarded guest output; feature_cmd.go +// renders it to stdout), so the last rollup in the child's log is the report the child +// printed, and the per-check lines are the ones in the block before it. +// +// The log is the child's MERGED output: the cli seam captures the child's stdout and stderr +// into ONE text, reunited a whole line at a time, so a report cannot be told apart by the +// stream it was written to — and must not be, because the runner has no other view of the +// child. (childReport reads that merged log, and only for a plan child.) +// +// A log with NO rollup carries NO report and therefore NO check outcome. That is the +// difference between a verdict and a guess: a build step's output, a deploy trace, or any +// other non-plan log must not be able to credit a PASS to a run that proved NOTHING +// (opencharly/charly#904). Before this rule the reader collected every report-SHAPED line +// in the log, and the run-level counters counted them without asking whether a report had +// been printed at all. +// +// A report whose step lines do NOT agree with its own rollup is not read: the rollup says +// "N steps" and the block must carry N step lines. A block that does not is carrying text +// the report did not print — a step's MESSAGE with a report-shaped line inside it, which +// the engine's truncate() permits because it bounds a message's length but does not flatten +// its newlines. Such a report credits NOTHING, and the phase says so. func parseCheckReport(logText string) checkReport { - rep := checkReport{Present: reportRollupRe.MatchString(logText)} - for _, line := range strings.Split(logText, "\n") { + lines := strings.Split(logText, "\n") + // The child's own report is the LAST one it printed. + last := -1 + steps := 0 + for i, line := range lines { + m := reportRollupRe.FindStringSubmatch(line) + if m == nil { + continue + } + last = i + steps, _ = strconv.Atoi(m[1]) + } + if last < 0 { + // No report: no check outcome. The absence is not a claim that a check was skipped. + return checkReport{} + } + // The report's step lines are the ones between the PREVIOUS rollup and this one. + start := 0 + for i := last - 1; i >= 0; i-- { + if reportRollupRe.MatchString(lines[i]) { + start = i + 1 + break + } + } + rep := checkReport{Present: true, Steps: steps} + for _, line := range lines[start:last] { m := checkOutcomeRe.FindStringSubmatch(line) if m == nil { continue } + rep.Printed++ if !isCheckKeyword(m[2]) { continue } @@ -143,13 +272,22 @@ func parseCheckReport(logText string) checkReport { Reason: strings.TrimSpace(m[5]), }) } + if rep.Printed != rep.Steps { + // The block does not agree with the report's own count, so its per-check lines are + // not the report's own. Nothing is credited from it. + rep.Checks = nil + return rep + } + rep.Readable = true return rep } // checkReportVerdict is the verdict a child's report implies, and the reason for it. // +// FAIL — the phase's report says a declared check FAILED. +// SKIP — the phase asserted NOTHING: no declared check passed (or the report could not +// be read, so nothing can be credited from it). // PASS — the phase ASSERTED something: at least one declared check ran and passed. -// SKIP — the phase asserted NOTHING: no declared check passed. // // The skip is NOT "one declared check among many did not run". A declared check that does // not apply to this substrate resolves to SKIP BY THE AUTHOR'S DESIGN (`skip: true`, @@ -165,35 +303,82 @@ func parseCheckReport(logText string) checkReport { // is a SKIP: a check that cannot run is not a check that passed (R7). func checkReportVerdict(rep checkReport) (string, string) { if !rep.Present { + // No report: the phase has nothing to say about itself. The RUN's verdict is + // decided by what the run proved, which is what the reports say. return verdictPass, "" } + if !rep.Readable { + // A report that does not agree with itself cannot say what the run proved, so + // nothing is credited from it (opencharly/plugin-check#134). + return verdictSkip, fmt.Sprintf("the child's report could not be read: %d step line(s) against the %d step(s) its own rollup counted", + rep.Printed, rep.Steps) + } var skipped []checkOutcome + var failed []checkOutcome passed := 0 for _, c := range rep.Checks { switch c.Status { case verdictSkip: skipped = append(skipped, c) + case verdictFail: + failed = append(failed, c) case verdictPass: passed++ } } + if len(failed) > 0 { + // A declared check FAILED. The report and the exit code are derived from the same + // step results, so a child that honoured its contract already exited 2 and this + // branch is unreachable for it. It closes the case where a child exits 0 and its + // own report says otherwise — where the record would carry `status: FAIL` for a + // check while the run read PASS (opencharly/plugin-check#134). + return verdictFail, fmt.Sprintf("declared check %s failed: %s", checkID(failed[0]), checkReason(failed[0])) + } if passed > 0 { return verdictPass, "" } if len(skipped) > 0 { - reason := skipped[0].Reason - if reason == "" { - reason = "no reason given" - } - id := skipped[0].ID - if id == "" { - id = skipped[0].Text - } - return verdictSkip, fmt.Sprintf("declared check %s did not run: %s", id, reason) + return verdictSkip, fmt.Sprintf("declared check %s did not run: %s", checkID(skipped[0]), checkReason(skipped[0])) } return verdictSkip, "the child's report carries no passing check: it asserted nothing" } +// checkID names a declared check: its id, or its text when the report printed no id. +func checkID(c checkOutcome) string { + if c.ID != "" { + return c.ID + } + return c.Text +} + +// checkReason is the message the report carried for a check, or an honest stand-in. +func checkReason(c checkOutcome) string { + if c.Reason != "" { + return c.Reason + } + return "no reason given" +} + +// stepVerdictFromReport applies a child's own report to its step, and returns the failure +// sentence when the report says a declared check FAILED. +// +// A SKIP sets the step's skip reason — the step completed but proved nothing, and the +// reason is what tells a reader that (opencharly/plugin-check#132). A FAIL is returned as +// the sentence, because the step FAILED: the report and the exit code come from the same +// step results, so a child that honoured its contract already exited 2, and this closes +// the case where a child exits 0 and its own report says otherwise +// (opencharly/plugin-check#134). A PASS is no objection: the step carries on. +func stepVerdictFromReport(rep checkReport, sk *stepResult) string { + switch verdict, reason := checkReportVerdict(rep); verdict { + case verdictSkip: + sk.SkipReason = reason + case verdictFail: + sk.OK = false + return reason + } + return "" +} + // stepStatus is the STEP's verdict. // // FAIL — the step failed (the child failed, or its log carried a failure). @@ -251,6 +436,11 @@ func runStatus(res *bedRunResult) string { // runPassedChecks is how many declared checks across the run resolved to PASS — the question // the run's verdict asks: did the run prove anything it declared? +// +// The counts read the steps' checks, and a step carries checks ONLY when its own report was +// READABLE (parseCheckReport credits nothing out of a log that printed no report, and +// nothing out of a report that disagrees with its own rollup). So a PASS can only ever be +// credited from a report the child actually printed and that agrees with itself. func runPassedChecks(res *bedRunResult) int { if res == nil { return 0 diff --git a/candy/plugin-check/bed_status_test.go b/candy/plugin-check/bed_status_test.go index 3b2463d..c67eec5 100644 --- a/candy/plugin-check/bed_status_test.go +++ b/candy/plugin-check/bed_status_test.go @@ -2,6 +2,9 @@ package check import ( "errors" + "go/ast" + "go/parser" + "go/token" "os" "path/filepath" "strings" @@ -103,6 +106,107 @@ func TestParseCheckReportWithoutAReport(t *testing.T) { } } +// TestParseCheckReportCreditsNothingWithoutARollup pins the rule that closes the +// false-PASS half of opencharly/plugin-check#134: a log that printed NO report carries no +// check outcome, whatever lines in it happen to be shaped like one. +// +// The reader used to collect every report-SHAPED line in the log and leave the counters to +// decide what to do with them, so a build step's output could credit a PASS to a run that +// proved NOTHING — the false green the whole verdict change exists to close. +func TestParseCheckReportCreditsNothingWithoutARollup(t *testing.T) { + const log = "building the image\n PASS check a check that never ran [ghost-check]\n" + rep := parseCheckReport(log) + if rep.Present { + t.Errorf("parseCheckReport() claimed a report in a log that carries no rollup") + } + if rep.Readable { + t.Errorf("parseCheckReport() called a log with no rollup readable") + } + if len(rep.Checks) != 0 { + t.Errorf("parseCheckReport() credited %d check(s) from a log that printed no report, want 0", len(rep.Checks)) + } +} + +// TestParseCheckReportReadsTheChildsOwnReport pins WHICH report the reader takes when a log +// carries more than one: the child's OWN, which is the LAST one it printed. +// +// The engine renders the report last — `live_cmd.go` renders it after any output forwarded +// from a nested venue, `feature_cmd.go` renders it to stdout — so the last rollup in the log +// is the report the child printed, and the per-check lines are the ones in that block. A +// log that carries an earlier report must not merge the two: the earlier block's checks +// belong to a different plan. +func TestParseCheckReportReadsTheChildsOwnReport(t *testing.T) { + const log = ` PASS check the forwarded venue's own check [guest-check] + +1 step: 1 passed, 0 failed, 0 skipped + PASS check the child's own check [host-check] + +1 step: 1 passed, 0 failed, 0 skipped +` + rep := parseCheckReport(log) + if !rep.Present { + t.Fatalf("parseCheckReport() did not see the report in:\n%s", log) + } + if !rep.Readable { + t.Fatalf("parseCheckReport() did not read the child's own report in:\n%s", log) + } + if len(rep.Checks) != 1 { + t.Fatalf("parseCheckReport() read %d checks, want 1 (the child's own block only): %+v", len(rep.Checks), rep.Checks) + } + if rep.Checks[0].ID != "host-check" { + t.Errorf("the read check = %q, want %q — the child's own report is the last one it printed", rep.Checks[0].ID, "host-check") + } +} + +// TestParseCheckReportRefusesABlockThatDisagreesWithItsOwnCount pins the report's own +// self-check: the rollup says "N steps" and the block must carry N step lines. +// +// A block that does not is carrying text the report did not print — a step's MESSAGE with a +// report-shaped line inside it, which the engine's truncate() permits because it bounds a +// message's length but does not flatten its newlines. Such a report credits NOTHING: a +// verdict must not rest on text the report did not print. +func TestParseCheckReportRefusesABlockThatDisagreesWithItsOwnCount(t *testing.T) { + const log = ` SKIP check a check that does not apply to this run [not-applicable] skip: true + PASS check a line out of a step's message [ghost] + +1 step: 0 passed, 0 failed, 1 skipped +` + rep := parseCheckReport(log) + if !rep.Present { + t.Fatalf("parseCheckReport() did not see the report in:\n%s", log) + } + if rep.Readable { + t.Errorf("parseCheckReport() read a block of %d step line(s) against a rollup of %d step(s)", rep.Printed, rep.Steps) + } + if len(rep.Checks) != 0 { + t.Errorf("parseCheckReport() credited %d check(s) from a block that disagrees with the report's own count, want 0", len(rep.Checks)) + } + verdict, reason := checkReportVerdict(rep) + if verdict != verdictSkip { + t.Errorf("an unreadable report = %q, want %q — nothing can be credited from it", verdict, verdictSkip) + } + if !strings.Contains(reason, "could not be read") { + t.Errorf("the reason = %q, want the report's unreadability named", reason) + } +} + +// TestARunIsNotAPassOnTextThatIsNotAReport is the RUN-level statement of the same rule, and +// the one the false green would have taken: a run whose only report-shaped line came from a +// log that printed no report has proved NOTHING, so it is a SKIP and not a PASS. +func TestARunIsNotAPassOnTextThatIsNotAReport(t *testing.T) { + const log = "building the image\n PASS check a check that never ran [ghost-check]\n" + rep := parseCheckReport(log) + res := &bedRunResult{OK: true, Step: []stepResult{ + {Name: "image-build", OK: true, Checks: rep.Checks}, + }} + if got := runStatus(res); got != verdictSkip { + t.Errorf("a run that proved nothing = %q, want %q — text that is not a report cannot credit a PASS", got, verdictSkip) + } + if got := runPassedChecks(res); got != 0 { + t.Errorf("a run credited %d passing check(s) from a log that printed no report, want 0", got) + } +} + // TestCheckReportVerdict pins the PHASE verdict. // // The defect (opencharly/charly#904): a phase whose only declared check was SKIPPED proved @@ -140,6 +244,51 @@ func TestCheckReportVerdict(t *testing.T) { } } +// TestCheckReportVerdictTreatsAFailedCheckAsAFail pins the other half of the verdict: a +// report that says a declared check FAILED is a FAIL, not a PASS (the first non-blocking +// observation on opencharly/plugin-check#133, fixed in opencharly/plugin-check#134). +// +// The ordinary case is already covered by the exit code — the report and the exit code are +// derived from the same step results, so a child that honoured its contract exited 2 — but +// the gap is real when a child exits 0 and its own report says otherwise. Then the record +// would carry `status: FAIL` for a check while the run read PASS. +func TestCheckReportVerdictTreatsAFailedCheckAsAFail(t *testing.T) { + verdict, reason := checkReportVerdict(parseCheckReport(liveMixedCheckReport)) + if verdict != verdictFail { + t.Errorf("a report whose check FAILED = %q, want %q", verdict, verdictFail) + } + if !strings.Contains(reason, "sag-binary") { + t.Errorf("the reason = %q, want the failed check named", reason) + } +} + +// TestStepVerdictFromReport pins how a report reaches its STEP: a SKIP sets the step's skip +// reason, a FAIL is returned as the failure sentence, and a PASS is no objection. +func TestStepVerdictFromReport(t *testing.T) { + skipped := stepResult{OK: true} + if fail := stepVerdictFromReport(parseCheckReport(liveSkippedCheckReport), &skipped); fail != "" { + t.Errorf("a report that says a check skipped returned the failure %q, want none — a skip is not a failure", fail) + } + if skipped.SkipReason == "" { + t.Errorf("a skipping report left the step with no reason") + } + if !strings.Contains(skipped.SkipReason, "not-applicable-here") { + t.Errorf("the step's skip reason = %q, want the skipped check named", skipped.SkipReason) + } + + failed := stepResult{OK: true} + fail := stepVerdictFromReport(parseCheckReport(liveMixedCheckReport), &failed) + if fail == "" { + t.Errorf("a report that says a check FAILED returned no failure sentence") + } + if failed.SkipReason != "" { + t.Errorf("a failing report set the step's skip reason %q — a failure is not a skip", failed.SkipReason) + } + if stepStatus(&failed) != verdictFail { + t.Errorf("a step whose report says FAIL = %q, want %q", stepStatus(&failed), verdictFail) + } +} + // TestRunStatusIsThreeState pins the RUN verdict: PASS / SKIP / FAIL, with the FAIL // winning over the SKIP. func TestRunStatusIsThreeState(t *testing.T) { @@ -350,3 +499,163 @@ func TestAggregateRosterDoesNotFoldAnUnprovenBedIntoTheGreen(t *testing.T) { t.Errorf("the roster error = %q, want the unproven bed named", err.Error()) } } + +// TestANonPlanChildCreditsNothingFromItsLog pins the rule that keeps a build's output out +// of the verdict: a REPORT is what a PLAN child prints, so a build step's log can never +// bank a check — not even when its output ends with a block that looks exactly like a +// report. +// +// MEASURED before the rule: such a block WAS credited, and the run read PASS while its +// plan step proved nothing (opencharly/plugin-check#135). +func TestANonPlanChildCreditsNothingFromItsLog(t *testing.T) { + forged := "building layer...\n" + + " PASS check the image builds [some-build-step] ok\n" + + "1 step: 1 passed, 0 failed, 0 skipped\n" + rep, skipped := childReport([]string{"box", "build", "some/image"}, forged) + if rep.Present { + t.Errorf("a build's log was read as a report: %+v", rep) + } + if len(rep.Checks) != 0 { + t.Errorf("a build's log credited %d check(s), want 0", len(rep.Checks)) + } + if skipped != 0 { + t.Errorf("a build's log inflated the inner skip count to %d, want 0", skipped) + } + // The run-level consequence: the run must read as a SKIP, not a PASS. + res := &bedRunResult{OK: true, Step: []stepResult{ + {Name: "image-build", OK: true, Checks: rep.Checks}, + {Name: "check-live", OK: true, SkipReason: "declared check x did not run: skip: true"}, + }} + if got := runStatus(res); got != verdictSkip { + t.Errorf("a run whose plan proved nothing = %q, want %q", got, verdictSkip) + } +} + +// TestAPlanChildReadsItsReport pins THAT a plan child's log is read for a report — the +// report the child printed, which is the last block in its log. +// +// The cli seam captures the child's two streams into ONE text (a whole line at a time), so +// the runner reads the merged log; the report is the last block in it because the engine +// renders the report last. +func TestAPlanChildReadsItsReport(t *testing.T) { + for _, argv := range [][]string{ + {"check", "live", "bed"}, + {"check", "box", "some/image"}, + {"check", "feature", "run", "dep"}, + } { + rep, _ := childReport(argv, liveSkippedCheckReport) + if len(rep.Checks) != 1 { + t.Errorf("%v read %d check(s) from the child's own report, want 1", argv, len(rep.Checks)) + } + } +} + +// renderersNotReachableAsAStepChild are the report renderers a bed run never launches as a +// step: the hidden build-scope feature-run engine, which the `charly box feature run` +// bridge invokes with its own args (CheckCmd.FeatureBox — `charly check __feature-box`). +// A new renderer that a bed run CAN launch belongs in planChildReporters instead — +// otherwise its report is ignored and a run that proved something reads as a SKIP. +var renderersNotReachableAsAStepChild = map[string]bool{ + "CheckFeatureBoxCmd": true, +} + +// TestEveryReportRendererIsAPlanChild holds the reader's table against the SOURCE: every +// command in this package that renders a step report must be a plan child, with the stream +// it renders on, and every entry in the table must render one. +// +// The table is the only place that says which children print a report, so it must not be +// able to drift from the code that prints them. +func TestEveryReportRendererIsAPlanChild(t *testing.T) { + renderers := reportRenderersInSource(t) + for command := range renderers { + if renderersNotReachableAsAStepChild[command] { + continue + } + found := false + for _, r := range planChildReporters { + if r == command { + found = true + } + } + if !found { + t.Errorf("%s renders a step report but is not a plan child — its report would be ignored", command) + } + } + for key, command := range planChildReporters { + if !renderers[command] { + t.Errorf("the table claims %q (%s) renders a report, but %s renders none", key, command, command) + } + } +} + +// reportRenderersInSource finds every command in this package that renders a step report, +// by reading the package's own source. +func reportRenderersInSource(t *testing.T) map[string]bool { + t.Helper() + fset := token.NewFileSet() + // parser.ParseDir is DEPRECATED (SA1019) and, worse, it ignores build tags when it + // associates files with packages. The files are read one by one instead, so the scan + // sees exactly the sources the compiler sees. + entries, err := os.ReadDir(".") + if err != nil { + t.Fatalf("reading the package's directory: %v", err) + } + renderers := map[string]bool{} + for _, entry := range entries { + name := entry.Name() + if entry.IsDir() || !strings.HasSuffix(name, ".go") { + continue + } + file, err := parser.ParseFile(fset, name, nil, 0) + if err != nil { + t.Fatalf("parsing %s: %v", name, err) + } + for _, decl := range file.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Recv == nil || len(fn.Recv.List) == 0 { + continue + } + command := receiverTypeName(fn.Recv.List[0].Type) + if command == "" { + continue + } + if rendersStepReport(fn.Body) { + renderers[command] = true + } + } + } + return renderers +} + +// receiverTypeName names a method's receiver type ("CheckLiveCmd" for (*CheckLiveCmd).Run). +func receiverTypeName(expr ast.Expr) string { + switch t := expr.(type) { + case *ast.StarExpr: + return receiverTypeName(t.X) + case *ast.Ident: + return t.Name + case *ast.IndexExpr: + return receiverTypeName(t.X) + case *ast.IndexListExpr: + return receiverTypeName(t.X) + } + return "" +} + +// rendersStepReport reports whether a function body renders a step report. +func rendersStepReport(body *ast.BlockStmt) bool { + found := false + ast.Inspect(body, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + ident, ok := call.Fun.(*ast.Ident) + if !ok || ident.Name != "reportSteps" { + return true + } + found = true + return true + }) + return found +}