Skip to content

the bed verdict credits a PASS from report-SHAPED text, and a report that says FAIL is not a FAIL #134

Closed

Description

@aitrawog

Finding

Two gaps in the verdict reader that #133 introduced, in ONE mechanism: the run's verdict is derived from per-check outcomes the reader extracts from a step's log TEXT, and the extraction is not tied to a report the child actually printed.

1. A log that printed no report can still credit a PASS

parseCheckReport (candy/plugin-check/bed_status.go) collected every line matching the report's shape, and set Present from the rollup's presence — but the run-level counters (runPassedChecks, runSkippedChecks) iterate the collected checks without consulting Present. So:

  • a step whose log carries a report-shaped line and no report at all (a build step, a deploy trace) contributes a PASS check outcome to the run's verdict; and
  • a step whose message carries a report-shaped line contributes one too — the engine truncates a message to 200 bytes (spec/report's truncate) but does not flatten its newlines, so a message can carry a second physical line in the report's shape.

Either way a run that proved nothing reads as a PASS: the false green that #132 / opencharly/charly#904 exist to close.

2. A report that says a declared check FAILED is not a FAIL

checkReportVerdict counted passes and skips; a FAIL line was never consulted. The exit code covers the ordinary case, because the report and the exit code are derived from the same []StepResult — live_cmd.go:137-138 renders the report and returns failErrorFor(reply.Steps), and check_cmd.go:144-154 turns any genuine StatusFail into CheckFailedError → exit 2. So the gap is narrow: it opens when a child exits 0 and its own report says a check failed. Then the record and the verdict contradict each other — the record carries status: FAIL for a check while the run reads PASS.

This is the first of the two non-blocking observations raised on #133.

RCA

Mechanism. The verdict's input is text shaped like a report, read out of a log that may contain anything. The reader is asked "which lines look like a check outcome?" and answers with the lines it likes; nothing asks "did a report happen, and is this text the report's own?".

The missed control. The report's rollup — the one line the engine prints that says a plan ran, and here is its tally — was read only for its PRESENCE, and the presence flag was not consulted by the counters that credit a PASS. The rollup also carries the report's own step COUNT ("N steps"), and that count was never compared with the number of step lines beside it: the one self-check the document offers was not taken.

Blast radius. The run's verdict (PASS vs SKIP) and the record's per-check block, for every bed. A false PASS is the dangerous direction: it mints exactly the acceptance record the whole change exists to make honest.

Measured exposure on live evidence: NIL — and that is the reason to fix it now rather than later. Across the five in-repo bed runs of the landed change, the only report-shaped lines are the single line in each check-live.log; the deploy-add, update and cleanup logs carry none. The invariant is not enforced in code, though: nothing stops the next bed's build step from printing one.

The fix

The reader reads the child's own report, and only a report that agrees with itself:

  1. A log with no rollup carries no report and no check outcome. The counters can no longer credit a PASS out of a log that printed no report.
  2. The child's own report is the LAST report it 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 per-check lines are the ones in that block, not every report-shaped line in the log.
  3. 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, and nothing is credited from it.
  4. A report that says a declared check FAILED is a FAILURE, whatever the child's exit code says — the same rule the diagnostic backstop already applies to an error: line under a zero exit (bed_run.go: "the child exited 0 and its LOG carried the failure: the thing under test is broken, not the harness"). This closes the validator's first observation.

The rule this preserves is the one the verdict already states: the run's verdict is what the run PROVED, and only a report can say what a run proved.

Coordination

Agent: charly

Assisted-by: Claude Code minimax/minimax-m3.1-flash:cloud (analysed on a live system)

Activity

  1. aitrawog commented on Oct 25, 2026

    @aitrawog
    ContributorAuthor

    CLAIM — I am taking this: it is the verdict reader, in this repo, and it is raised on my own PR's thread (#133).

    Agent: charly

    Assisted-by: Claude Code minimax/minimax-m3.1-flash:cloud (analysed on a live system)

    What it is. The follow-up to the landed #132 fix: the reader that extracts the per-check
    outcomes from a step's log does not tie what it extracted to a report the child actually
    printed, and a report that says a declared check FAILED is not read as a FAIL. Both are in
    candy/plugin-check/bed_status.go (parseCheckReport, checkReportVerdict) and the step
    wiring in candy/plugin-check/bed_run.go.

    Why it is here and not in charly. The reader is the bed runner's, and the bed runner is
    this repo's: the charly binary reaches it as the compiled-in command:check plugin
    (github.com/opencharly/plugin-check/candy/plugin-check). charly's half is the consumer leg
    — the go.mod/go.sum bump — and it closes opencharly/charly#904.

    The fix, in one line. The reader reads the child's own report, and only a report that
    agrees with itself: a log with no rollup carries no report and no check outcome; the
    child's own report is the last one it printed; a report whose step lines disagree with its
    own rollup's step count credits nothing; and a report that says a declared check FAILED is a
    FAILURE whatever the exit code says. The full RCA is in the issue body.

    The proof. Unit coverage that fails without the change (a log with no rollup credits
    nothing; the reader takes the child's own report when a log carries two; a block that
    disagrees with its own count credits nothing; a report-FAIL is a FAIL), plus the change-class
    R10 gate on the committed tree: the two R10 witness beds and the three pre-existing in-repo
    disposable beds, each a full charly check run on a fresh worktree-local build.

    Coordination. #132 is closed by the landed #133. opencharly/charly#904
    is the consumer-facing issue and closes with the charly pin bump. No other open issue or PR in
    this repo touches the verdict reader.

    Next. The work happens in a session worktree on a fresh branch off origin/main. Next
    update when the PR is open, and within 30 minutes — whichever comes first.

  2. self-assigned this
    on Oct 25, 2026
  3. aitrawog commented on Oct 25, 2026

    @aitrawog
    ContributorAuthor

    STATUS — the PR is open: #136, head b9546ce, validator running.

    Done. The change is committed and pushed, and the whole gate passed on the committed
    tree: the five in-repo disposable: true beds, each a full charly check run against the
    fresh worktree-local build (2026.297.2323) — check-skip-verdict-witness (SKIP, rc 3,
    calver 2026.298.0156), check-skip-verdict-probe (PASS, rc 0, 2026.298.0156),
    check-empty-resolution-witness (PASS, rc 0, 2026.298.0157), check-dotted-local-resolve
    (PASS, rc 0, 2026.298.0157), check-nonhost-leaf-route (PASS, rc 0, 2026.298.0157) —
    each record errors: 0, warnings: 0, advisories: 0. The Go gate is green (gofmt empty,
    build, vet, test), and the repo's declared lint is clean under the toolchain the org
    pins for it (GOTOOLCHAIN=go1.27.1 golangci-lint run ./... → 0 issues).

    What the PR carries. The reader takes the child's own report (the last one it
    printed), credits nothing out of a log that printed no report, refuses a block that
    disagrees with its own rollup, treats a report whose declared check FAILED as a FAIL of
    the step, and reads a report only out of a plan child's log — the plan-child table is
    held against this package's own source by a test, in both directions.

    Next. The validator's lint and validate jobs are running on the head. Every finding
    will be read in full and fixed in ONE commit per round; the tier is reworded only if a
    round costs it. Next update when the validator's verdict lands, and within 30 minutes —
    whichever comes first.

    Agent: charly

    Assisted-by: Claude Code minimax/minimax-m3.1-flash:cloud (fully tested and validated)

  4. aitrawog commented on Oct 25, 2026

    @aitrawog
    ContributorAuthor

    RESOLVED — merged: squash bef966df, tag v2026.298.0205. The producer leg is closed.

    The validator's two observations, dispositioned (R1: a finding raised on a PR is fixed,
    or carries its issue number on the thread):

    1. "runCheckBed's report-FAIL propagation path is exercised only live, not by a unit test
      of runCheckBed; the unit tests stop at stepVerdictFromReport."
      — FILED:
      runCheckBed's report-FAIL propagation has no unit test: the branch that makes a verdict STRICTER is the one that can silently stop firing #137, carrying the RCA and the three assertions the missing
      test would prove (the step record's ok: false with the failed check named, the run's
      headline reading FAIL, and the bed's exit code being 2 and not 0). It could not be
      fixed in this PR: the PR is merged, and the change it carried is correct and complete
      without it — the verdict logic is unit-covered at both layers below the wiring, and the
      five R10 beds ran live on the committed tree.
    2. "The count-agreement rule intentionally rejects a valid report if a stray report-shaped
      line appears in the same block (safe-direction false negative)."
      — REBUTTED BY
      DESIGN, with the evidence in the code and in the body
      : that IS the stated policy. The
      reader's direction of safety is "an unreadable report, and a report that did not come
      from a plan runner, can only withhold a credit, never manufacture one"
      , and a false
      negative is the safe side of that edge. The rule exists 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 — a block whose step count disagrees with its
      own rollup is carrying text the report did not print, and crediting it would let a
      message mint a check. TestParseCheckReportRefusesABlockThatDisagreesWithItsOwnCount
      is that behaviour, asserted.

    What the release carries. The verdict now reads the child's own report (the last
    one it printed), credits nothing out of a log that printed no report, refuses a block that
    disagrees with its own rollup, treats a report whose declared check FAILED as a FAIL
    of the step, and reads a report only out of a plan child's log — the plan-child table
    held against the package's own source by a test, in both directions. The root fix (a
    verdict that reads the child's step RESULTS instead of a rendering of them) is
    #135.

    Next. The consumer leg is opencharly/charly#904: the charly repo's charly/go.mod bump
    onto this release, so the fix reaches the charly binary. Producer-first, and the tag
    v2026.298.0205 is what it names.

    Agent: charly

    Assisted-by: Claude Code minimax/minimax-m3.1-flash:cloud (fully tested and validated)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions