Skip to content

smoke.mjs: a declared GAP cannot survive a fatal assert, so an uncovered property reads as covered #720

Description

inbound/smoke.mjs declares an uncovered property with GAP ... NOT COVERED by this run, and
reports the count on the final line, so that a property the run could not check is visible instead
of silently passing. That is the design, and #692 relies on it explicitly: "an uncovered property is
visible rather than passing silently ... The run tells the truth about what it did not check."

It only tells that truth on the happy path. Every gap() call site is inline and downstream of
fatal asserts in the same leg, and fail() ends the process, so one failed probe erases the coverage
ledger for everything behind it. The absence of a GAP line is then ambiguous between "this property
was covered" and "the run never reached the declaration", which is exactly the distinction the GAP
mechanism exists to remove.

Measured, two real tag-deploy runs of v1.6.0

Both runs had the SAME uncovered property: POSTERN_NO_ORGANIZE_TOKEN empty in the step env, so the
refusal arm was uncoverable in both, pending #338 in crew-secrets.

Run 37945846771 (red, leg 6 grant probe got a 401 from a mismatched token):

6. seen / flags / move on the message this run created
FAIL  the organize probe answers 200 (granted) or 403 (refused), not something else
      { "status": 401, "json": { "ok": false, "error": "unauthorized" } }
##[error]Process completed with exit code 1.

Run 37946760315 (green, same step, correct token):

6. seen / flags / move on the message this run created
  ok  the organize probe answers 200 (granted) or 403 (refused), not something else
GAP   a token that must NOT organize is refused (set POSTERN_NO_ORGANIZE_TOKEN to cover it) -- NOT COVERED by this run
  ...
PASS: 55 checks green. 1 property(ies) NOT COVERED -- see GAP lines above.

Line counts over the two job logs, so the denominator is explicit rather than asserted:

GAP lines NOT COVERED PASS: summary
red 37945846771 0 0 0
green 37946760315 2 2 1

The red run published no coverage statement of any kind while carrying an identical uncovered
property. A reader grepping that log for GAP or NOT COVERED gets zero hits and no summary, which
reads the same as a fully covered run.

Mechanism

  • fail() (line 121-125) ends with process.exit(1). A hard exit, so there is no unwinding and a
    try/finally around the legs would NOT recover the ledger; only process.on("exit", ...) would.
  • assert() (line 126) routes every failure into fail(), so any assert is fatal.
  • The leg 6 grant probe asserts at line 450 that the organize probe answered 200 or 403.
  • Three of the four gap() call sites sit downstream of that one assert: line 478 (the refusal
    arm, which is the The organize regression guard cannot fire: both smoke secrets are unset #692 regression guard for GHSA-xgxm), line 492 (the seen/flags/move round trip),
    and line 542 (folder counts move on a file).
  • The summary at line 682 that reports the gaps count is downstream of everything.

So a single wrong token value suppressed three declared coverage gaps plus the whole coverage
summary, and nothing in the log said so.

Why this is not #692

#692 is OPEN and correctly scoped to the secret VALUES being absent, and its step 5 is "confirm a
normal run reports 0 GAPs". This issue is about the GAP mechanism itself being unable to report from
behind a fatal assert, which is the premise #692 leans on when it argues #691 was safe to merge
before the secrets existed. That premise is sound only while every upstream probe passes. Fixing
#692 does not fix this, and this was invisible until a run failed upstream of a gap site.

Proposed fix, two parts

  1. Declare the coverage intent BEFORE the probes run. The uncovered set is a pure function of
    config, known at startup: POSTERN_NO_ORGANIZE_TOKEN empty means the refusal arm is uncoverable
    whatever happens later. Printing the declarations up front makes them independent of control flow.
  2. Reconcile the ledger on exit regardless of outcome. Move the summary into a
    process.on("exit", ...) handler, so a failing run still publishes its GAP count. A finally
    will not do it, given the process.exit(1).

Together these give the property the mechanism was supposed to have: the coverage statement is
emitted on every path, not only the green one. Doing only (2) would still lose a gap whose gap()
call was never reached, which is why (1) carries the declaration.

Verification, and this is the part that matters

Reproduce the red run deliberately: point POSTERN_SMOKE_ORGANIZE_TOKEN at a value the worker does
not know, so leg 6's grant probe answers 401 again. The run MUST still exit non-zero AND still print
the refusal-arm GAP and a coverage summary. A fix that makes the run green, or that drops the GAP
again, is not a fix. Per the house rule, watch it produce the correct declaration on the real failure
path before trusting it.

Note for whoever picks this up: 401 and 403 are not interchangeable here. 401 means the bearer matched
no configured token, 403 means it resolved to a scope and was refused at the gate. organizeProbe
falls back to cfg.readToken when POSTERN_ORGANIZE_TOKEN is empty (line 446), and that fallback
produces 403, not 401. That difference is what proved the red run was a token-value mismatch rather
than a skipped leg, and a fix should keep both statuses distinguishable in the output.

Found while reviewing #719 (the v1.6.0 release) and reading the tag deploy runs.

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

Metadata

Metadata

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