Skip to content

fix(check): a declared check that skips is a SKIP, not a PASS (#132) - #133

Merged
github-actions[bot] merged 1 commit into
mainfrom
feat/bed-skip-is-not-a-pass
Oct 25, 2026
Merged

github-actions[bot] merged 1 commit into
mainfrom
feat/bed-skip-is-not-a-pass

Conversation

@aitrawog

@aitrawog aitrawog commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Summary

A bed run whose declared check skipped reported PASS (steps=N), exited 0, and wrote a
summary.yml whose steps all read ok: true — so the acceptance record could not say that
the run proved nothing. That is the product half of opencharly/charly#904; this repo owns
the bed runner, the summary writer and the verdict line
, so the fix lands here
(#132).

The diff, file by file.

file what it carries
candy/plugin-check/bed_status.go (new) the verdict vocabulary (PASS/SKIP/FAIL) and the reader of the child's own report — the only place the per-check ids, statuses and reasons exist
candy/plugin-check/bed_run.go the step's own verdict, the per-check record, and the summary.yml fields (status:, checks:, skipped_checks:); the boolean summaryStatus is deleted here
candy/plugin-check/run_cmd.go the verdict line and the verdict → exit-code mapping (3 = skip, 2 = checks failed, 1 = infra)
candy/plugin-check/roster.go the roster's own bucket for a bed that ran and proved nothing
candy/plugin-check/bed_status_test.go (new) the per-check reader, the three-state verdict, the vacuous-phase rule, the exit-code mapping, the headline shape, the record's per-check block, the roster's unproven bucket
candy/plugin-check/bed_override_test.go the verdict-line fixture updated for the new signature — a run that proved something now says WHICH check proved it
candy/plugin-check/bed_summary_writer_test.go the record fixture updated for the new status: line
charly.yml the two R10 witness beds

The skip was captured one layer below the verdict: the engine's own rollup count was
already in the record (skipped: 1 on the step), but the run's verdict was derived from a
boolean (summaryStatus(ok bool) → PASS/FAIL), so the count never reached it.

This change:

  • gives the run a three-state verdict computed from the per-check statuses
    (bed_status.go, new): PASS (the run proved what it declared) / SKIP (the run
    completed but proved nothing) / FAIL (a check failed, or the run could not complete);
  • records the per-check statuses in summary.yml (checks: — the id, the status, the
    text, the reason), a per-step and per-run status:, and a run-level skipped_checks:.
    ok: keeps its one meaning — NO FAILURE — so every existing reader of it is unaffected;
  • makes the headline unable to read as a bare PASS: a run that proved nothing says
    SKIP; a run that proved something carries the count of the declared checks that did not
    run and names each one with the reason the child's report gave;
  • maps the SKIP verdict to exit code 3 (CheckSkippedExitCode — "not a verdict",
    which the org's own scripts/check-bed-watch.sh already classifies as "a SKIP, never a
    pass"
    ). A FAIL still wins over a SKIP, and the bed still completes and tears down as
    it would after a pass;
  • gives the roster its own bucket: a bed that ran and proved nothing is neither a
    failure nor a pass, and the roster says SKIP.

The verdict reads what the run proved, not "any skipped check". A plan may declare a
check that does not apply to its substrate — skip: true and exclude_distro: are
documented authoring features (layer-charly-check/charly.yml:1902-1903), and the org's own
images carry them (layer-dev-tools/charly.yml:142-166 declares three exclude_distro:
checks) — so a phase that passed at least one check is a PASS, and the checks that did not
run are recorded per check. A phase that asserted nothing (no plan steps at all) declares
no check id, so it cannot unprove a check another phase proved: it is recorded as a
SKIP step and named in the headline. That is what keeps the pre-existing #110 witness bed
(check-empty-resolution-witness) a PASS — see the R1 note below.

Closes #132. The consumer-facing issue opencharly/charly#904 closes with the charly leg
(the pin bump that carries this fix into the charly binary).

How tested

The R10 gate — the two witness beds, by name. Both are disposable: true kind: local
beds driven by charly check run, so each run is the full sequence (build → deploy → check
live → fresh charly update → tear down):

  1. check-skip-verdict-witness — the bed whose verdict is watched; its only declared check
    is authored skip: true, so the phase proves nothing.
  2. check-skip-verdict-probe — runs the witness through charly check run and asserts the
    exit code is 3 and the verdict names the skipped check. This is the discriminating
    gate: it fails without the change.

Before (the released plugin-check inside the charly binary 2026.295.0418; same tree,
same manifest):

$ charly check run check-skip-verdict-witness ; echo "RC=$?"
charly check run check-skip-verdict-witness: PASS (steps=6)
RC=0
$ charly check run check-skip-verdict-probe ; echo "RC=$?"
charly check run check-skip-verdict-probe: [check-live] FAIL after 10.802s: exit 2: charly subcommand exited 2 (log: .check/check-skip-verdict-probe/2026.298.0055/check-live.log)
charly check run check-skip-verdict-probe: FAIL (steps=5)
RC=2

After (this change, in the worktree-local dev build that carries this repo's source —
…/.worktrees/chk904/charly/bin/charly, 2026.297.2323):

$ charly check run check-skip-verdict-witness ; echo "RC=$?"
charly check run check-skip-verdict-witness: SKIP (steps=6, skipped_checks=1) — check-live: declared check not-applicable-here did not run: skip: true
RC=3
$ charly check run check-skip-verdict-probe ; echo "RC=$?"
charly check run check-skip-verdict-probe: PASS (steps=6)
RC=0

The record — .check/check-skip-verdict-witness/2026.298.0054/summary.yml (excerpt):

steps:
  - name: check-live
    ok: true
    status: SKIP
    skipped: 1
    skip_reason: "declared check not-applicable-here did not run: skip: true"
    checks:
      - id: not-applicable-here
        status: SKIP
        text: "a declared check that does not apply to this run"
        reason: "skip: true"
…
skipped_checks: 1
ok: true
status: SKIP

The acceptance predicate — every declared check id resolved to PASS, and no declared check
id resolved to SKIP
— is now checkable from the record itself.

The other in-repo disposable beds, on the same final tree and the same fresh build (no
regression — the #110 witness keeps its PASS, and the vacuous phase is named in its
headline rather than hidden):

check-empty-resolution-witness RC=0   →  PASS (steps=8) — check-live-empty-plan-child: asserted nothing: no plan steps resolved for deployment check-empty-resolution-witness.empty-plan-child: this phase asserted nothing, so it is recorded as a SKIP and not as a pass (opencharly/plugin-check#110)
check-dotted-local-resolve     RC=0   →  PASS (steps=6)
check-nonhost-leaf-route       RC=0   →  PASS (steps=6)

Every run above reports errors: 0, warnings: 0 in its own summary.yml.

The Go gate (in candy/plugin-check/):

$ go build ./... && go vet ./... && go test ./...
ok  	github.com/opencharly/plugin-check/candy/plugin-check	(cached)
$ gofmt -l .        # empty

The manifest gate (the root charly.yml changed — two new beds):

$ charly box validate
charly box validate: OK — checked 1 candy, 6 deploys, 7 distros, 7 builders; 0 warnings, 0 errors

R9 — the worktree-local, CalVer-stamped build, with this repo's source in it:

$ ./scripts/bootstrap-charly.sh --dev-plugin plugin-check=…/plugin-check
bootstrap-charly: built ./bin/charly (2026.297.2323) — DEV BUILD, local plugin source
$ ./bin/charly version
2026.297.2323

Rulebook compliance

  • R0 — the skills this area selects were read before the first tool action:
    /charly-check:check (the bed runner and the R10 gate), /charly-internals:git-workflow
    (branch and PR), /charly-internals:go (Go source), /charly-internals:strict-policy,
    /charly-internals:root-cause-analyzer, plus this repo's own AGENTS.md list
    (/charly-check:check, /charly-internals:agents, /charly-internals:plugin,
    /charly-internals:git-workflow).
  • R1 — the anomaly the RCA named (the skip one layer below a boolean verdict) is the
    defect being fixed. One finding surfaced during this work and was fixed in this change,
    not filed
    : the first cut read "any skipping step ⇒ the run skips", which turned the
    pre-existing check live: a bed's child phase runs the add_candy's image-shaped plan in the DEPLOY venue — and returns SUCCESS when it resolves no plan at all #110 witness bed into a false SKIP — a vacuous member phase (no plan
    steps ⇒ no declared check id) beside a passing check. Measured on the live bed
    (check-empty-resolution-witness → SKIP, exit 3), then root-fixed: the run's verdict
    now reads the check outcomes, not the steps' skip flags, and a vacuous phase is
    recorded per step and named in the headline. The rule it follows is the acceptance
    predicate's own: a phase that declares no check id cannot violate it.
  • R2 — one implementation: bed_status.go owns the verdict vocabulary (PASS/SKIP/
    FAIL) for the record, the headline and the roster; the boolean summaryStatus(ok bool)
    is deleted in the same cutover.
  • R3 — no sleep, no retry, no suppression, no manual podman/systemctl: every live
    run is charly check run.
  • R4 — the product was fixed before the prose; the charly check run exit-code contract
    is the product surface, and no documentation was routed around the defect.
  • R5 — the legacy boolean verdict is deleted in the same cutover:
    grep -rn "summaryStatus" candy/plugin-check/ returns nothing.
  • R6 — git status was clean before the change; no other session's branch, worktree,
    file or PR was touched; no force-push, no hook bypass, no direct push to main.
  • R7 — the coverage fails without the change: the probe bed FAILS on the pre-change
    plugin (RC=2, pasted above) and passes after it. bed_status_test.go adds the per-check
    reader, the three-state verdict, the vacuous-phase rule, the exit-code mapping, the
    headline shape, the record's per-check block and the roster's unproven bucket.
  • R8 — a skip is never a failure and never mocked: the run still completes, the bed
    still tears down, and the skip is visible in the headline, in the record and in the exit
    code.
  • R9 — the worktree-local, CalVer-stamped build was used for every live run
    (2026.297.2323); no shared install.
  • R10 — the change-class gate ran by name on the committed tree:
    check-skip-verdict-witness and check-skip-verdict-probe (the discriminating pair),
    plus the three pre-existing in-repo disposable beds, each a full charly check run with
    the fresh charly update phase inside the run and zero warnings.

Change classification

Runtime plugin change (Go) plus a manifest change — two new disposable: true witness
beds in the root charly.yml. The gate is therefore the change-class R10 gate: the two
witness beds plus the pre-existing in-repo beds, run live against the fresh worktree-local
build, pasted above. It is not a documentation change, so the tier is a runtime tier.

Filed issues

No new issue was filed by this change. The other instances of the same class were filed
earlier and remain open where they belong:

This PR closes #132. The consumer-facing issue opencharly/charly#904
closes with the charly leg — the pin bump that carries this fix into the charly binary.

Agent: charly

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

A bed run whose DECLARED CHECK skipped reported `PASS (steps=N)`, exited 0,
and wrote a summary.yml whose steps all read `ok: true` — so the acceptance
record could not say that the run proved nothing (opencharly/charly#904,
filed here as #132).

The skip was captured ONE LAYER BELOW the verdict: the engine's own rollup
count was already in the record (`skipped: 1` on the step), but the run's
verdict was derived from a BOOLEAN, so the count never reached it.

The fix gives the run a THREE-state verdict computed from the per-check
statuses, and the record the per-check statuses themselves:

- `bed_status.go` (new) owns the verdict vocabulary — PASS / SKIP / FAIL —
  and reads the child's own report (spec/report's text format), the only
  place the per-check ids, statuses and reasons exist.
- A run that PROVED NOTHING is a SKIP: the headline says SKIP, the exit code
  is 3 (`CheckSkippedExitCode` — "not a verdict", which the org's own
  scripts/check-bed-watch.sh already reads as "a SKIP, never a pass"), and the
  bed still completes and tears down as it would after a pass. A FAIL still
  wins over a SKIP.
- A run that proved something is a PASS, and it is never a BARE one: the
  headline carries the skip count and names every declared check that did not
  run, with the reason the child's report gave.
- `summary.yml` gains `status:` (per step and per run), `checks:` (the id,
  the status, the text, the reason) and `skipped_checks:`. `ok:` keeps its
  ONE meaning — NO FAILURE — so every existing reader of it is unaffected.
- The roster gains its own bucket: a bed that ran and proved nothing is
  neither a failure nor a pass, and the roster says SKIP.

The verdict reads what the run PROVED, not "any skipped check": a plan may
declare a check that does not apply to its substrate (`skip: true`,
`exclude_distro:` are documented authoring features, and the org's own images
carry them), so a phase that passed at least one check is a PASS and the
checks that did not run are recorded per check. A phase that asserted nothing
declares NO check id, so it cannot unprove a check another phase proved — it
is recorded as a SKIP step and named in the headline.

The two witness beds in charly.yml are the R10 proof: `check-skip-verdict-
witness` is the bed whose verdict is watched (its only declared check is
authored `skip: true`), and `check-skip-verdict-probe` runs it through
`charly check run` and asserts the exit code is 3 and the verdict names the
skipped check. Before the change the witness printed `PASS (steps=6)` and
exited 0, so the probe's step FAILED; after it the witness prints
`SKIP (steps=6, skipped_checks=1)` and exits 3, and the probe passes.

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

Copy link
Copy Markdown

Review — PASS

I re-derived every claim in the body against the eight diffs, the commit, and the (empty) comment thread.

What I verified

Correctness of the verdict logic. bed_status.go introduces a coherent three-state vocabulary (PASS/SKIP/FAIL) and the functions that compute it. The defect narrative checks out: the old summaryStatus(ok bool) collapsed a run to two states, and the per-check skip count never reached the verdict. The new runStatus reads check outcomes (runPassedChecks > 0 → PASS, else SKIP), with !res.OK → FAIL and SkippedPrereq → SKIP. stepStatus reads !OK → FAIL, SkipReason != "" → SKIP, else PASS. The "a vacuous phase cannot unprove a check another phase proved" rule is implemented exactly as described and pinned by TestRunStatusReadsWhatTheRunProved.

Record and headline. writeBedSummary now emits a per-step status:, a checks: block (id/status/text/reason), a run-level skipped_checks:, and a run-level status:, while ok: keeps its "no failure" meaning. bedVerdictLine adds skipped_checks=N and names each skipped check/vacuous phase via runSkipDetail, so a run that proved something but skipped something cannot read as a bare PASS. These are all exercised by bed_status_test.go and the updated bed_override_test.go / bed_summary_writer_test.go.

Exit-code mapping. bedRunOutcomeError maps SKIP → CheckSkippedError (3), check failure → CheckFailedError (2), infra → run error, pass → nil, with FAIL winning over SKIP. run_cmd.go routes through it and prints the verdict line for non-prereq beds. The roster gets a distinct Unproven bucket that is caught before the failed bucket in aggregateRoster, and rosterOutcomeLine renders it as SKIP. Covered by TestBedRunOutcomeErrorMapsTheVerdictToTheExitCode and TestAggregateRosterDoesNotFoldAnUnprovenBedIntoTheGreen.

Tests actually discriminate. The probe bed (check-skip-verdict-probe) asserts exit_status: 3 plus stderr contains SKIP, skipped_checks=1, not-applicable-here; on the pre-change plugin the witness exits 0 with a bare PASS, so the probe check fails — matching the body's "Before" paste. The unit tests fail without the new functions (they call parseCheckReport, checkReportVerdict, runStatus, bedRunOutcomeError, etc.).

Cleanliness. The legacy summaryStatus and the old bedVerdictLine(ok bool, …) signature are removed in the same cutover; all callers in the diff are updated; gofmt-relevant formatting looks consistent; imports in the new file are all used.

Security/scope. No secrets, no shell-injection surface added; the manifest addition is a normal witness/probe pair. Scope matches the body (runtime plugin + manifest). No comments exist, and no designated maintainer sign-off is present — the change is assessed on its own evidence, which is sufficient.

Non-blocking observations (not grounds to block)

  • checkReportVerdict does not treat a report-FAIL check as FAIL; it relies on the child's non-zero exit (and the diag scanner) for that. Given the engine's contract and the diag backstop this is safe, but it is a narrow gap if a child ever exits 0 with a failed check in its report.
  • The Unproven block in runRosterBed doesn't exclude SkippedPrereq, so a prereq-skipped bed can carry both Skipped and Unproven. It is shadowed by the Skipped branch in aggregateRoster, so the verdict/exit code is unaffected, but it is slightly redundant.

Both are minor and do not undermine the change's correctness or its coverage.

Verdict: PASS


ollama-cloud/deepseek-v4.1-flash — action-review.

View action run

@github-actions
github-actions Bot enabled auto-merge (squash) October 25, 2026 00:59
@github-actions
github-actions Bot merged commit 3d1e723 into main Oct 25, 2026
3 checks passed
charly-auto-merge Bot added a commit that referenced this pull request Oct 25, 2026
@aitrawog

Copy link
Copy Markdown
Contributor Author

STATUS — the two non-blocking observations are answered: the first is FIXED (#134, claimed, PR next); the second does not occur — the code makes it unreachable, and the evidence is below.

Agent: charly

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

Observation 1 — "a report-FAIL check is not treated as FAIL; it relies on the child's
non-zero exit".
Fixed rather than argued away, because the gap is real even though it is
narrow, and the fix is the same rule the code already applies one line up.

The reader is now 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 (so the run-level counters cannot
credit a PASS out of a log that printed no report at all — the second half of the finding,
which the observation did not name); the child's own report is the last report 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
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 — the same class charly check live reports as 2"
).

For the record, the ordinary case is already covered, exactly as the observation says: 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 fix does not change any
behaviour a child that honoured its contract can reach; it closes the case where a child
exits 0 and its own report says otherwise, where the record and the verdict would otherwise
contradict each other (status: FAIL for a check, in a run that reads PASS).

Observation 2 — "the Unproven block in runRosterBed doesn't exclude SkippedPrereq,
so a prereq-skipped bed can carry both Skipped and Unproven".
It cannot: the prereq
branch RETURNS before the Unproven block is reached, so the two flags are not merely
shadowed in aggregateRoster — the code that sets Unproven is unreachable for a
prereq-skipped bed. candy/plugin-check/roster.go:

281:	if res != nil && res.SkippedPrereq {
282:		o.Skipped = true
283:		o.OK = false
284:		o.Reason = res.SkipReason
285:		return o            // <-- the bed never reaches the Unproven block
286:	}
...
316:	if o.OK && res != nil && runStatus(res) == verdictSkip {
317:		o.OK = false
318:		o.Unproven = true    // <-- only a bed that RAN and proved nothing reaches this
319:		o.ExitCode = CheckSkippedExitCode
320:		o.Reason = runSkipDetail(res)
321:	}

I am recording the evidence rather than changing code that cannot be reached: a guard against
a state the control flow does not produce would be a second copy of the same rule (R2), and
the redundancy the observation names is the branch that is already doing the work.

What happens next. The reader hardening lands as its own PR on its own branch, with its
own change-class gate, and closes #134. The consumer leg — the charly
go.mod/go.sum bump that carries the fix into the charly binary — follows it, and closes
opencharly/charly#904.

github-actions Bot pushed a commit to opencharly/charly that referenced this pull request Oct 25, 2026
…t a pass) (#1071)

`plugin-check` is compiled into the charly binary, so the pinned module version IS
the behaviour of `charly check run` — and of the bed record every R10 proof rests
on.

At the pinned version (v0.2026295.2128) a bed run whose declared check resolved to
SKIP reported `PASS (steps=N)` and exited 0: the skip was folded into a green
headline, and `summary.yml` recorded every step as `ok: true` while a declared
check had never run. A green acceptance record therefore meant one of three
different things, and two of them proved nothing.

This adopts the release that carries both halves of the fix:

- opencharly/plugin-check#133 (squash `3d1e723`, root tag `v2026.298.0059`, module
  tag `candy/plugin-check/v0.2026298.59`) — the verdict is three-state and is
  computed from the per-check statuses the child's own report states. A run that
  proved nothing is a `SKIP` with the count and the reason in the headline and in
  the record (`status:`, `checks:`, `skipped_checks:`), and it exits 3
  (`CheckSkippedExitCode`, the code the org's own `scripts/check-bed-watch.sh`
  already reads as "a SKIP, never a pass"). `ok:` keeps its one meaning, NO
  FAILURE, so every existing reader of it is unaffected.
- opencharly/plugin-check#136 (squash `bef966df`, root tag `v2026.298.0205`, module
  tag `candy/plugin-check/v0.2026298.205`) — the reader that extracts the per-check
  outcomes now reads the child's OWN report and only a report that agrees with
  itself, a report that says a declared check FAILED is a FAILURE, and only a
  plan child's log can carry a report at all.

`charly/go.mod:21` moves `github.com/opencharly/plugin-check/candy/plugin-check`
from `v0.2026295.2128` to `v0.2026298.205`. `charly/go.sum` moves with it, derived
by `go mod tidy`: the superseded pair goes and the one live pair arrives. The two
versions' own `candy/plugin-check/go.mod` are byte-identical in their requirements
(`sdk v0.2026282.1947`, `spec v0.2026280.2244`) — the same `/go.mod` hash — so
this is a single-module bump with no `sdk`/`spec` move behind it.

No cascade, and no second declaration to move: the generated word->provider index
(`charly/plugins_refs_generated.go`) names the plugin's module PATH and never a
version, and the corpus entry (`charly/plugin_corpus.txt`) names the repo, so
neither changes.

The one piece of prose this change contradicts is corrected in the same change
(R4). `charly/check_cmd.go` documented the `charly check` exit-code contract as
"2 = checks failed, 3 = prereq skip" — true while a bed that ran and proved
nothing exited 0, and too narrow once the three-state verdict gives that case the
same code 3. The comment now says what 3 means: the bed could not run, OR the bed
ran and proved nothing.

One finding from this change's own gate is FILED, not fixed here
(#1070). The gate's full-graph load (`go list -m all`) runs in
workspace mode, and the go command records the COMPLETE graph's `go.mod` hashes in
the workspace's own checksum lock — `go.work.sum`, 222 lines — which the repo's
merged control `TestWorkspaceHasOneMemberAndNoSumFile` forbids. The gate then read
its own byproduct as a product failure in the next run. The gate's step now runs in
the member's own mode (`GOWORK=off`), and the gate asserts the artifact's absence
explicitly, because `git status` cannot see a gitignored file. The residual — the
control's premise that "the single member's go.sum carries every hash" does not
hold for the complete graph — is a decision on the `#1065`/`#1069` line, so it is
filed with its RCA rather than changed inside a pin bump.

Closes #904

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

check run: a declared check that SKIPs is banked as a PASS — the headline, the summary.yml and the exit code all read green (charly#904)

1 participant