-
Notifications
You must be signed in to change notification settings - Fork 0
feat(commits): report artifacts a commit claims but never advanced (REQ-315) - #888
Merged
Merged
feat(commits): report artifacts a commit claims but never advanced (REQ-315) #888avrabe merged 1 commit into
avrabe merged 1 commit into
Conversation
...EQ-315) A trailer is a LINK, not a state transition. `rivet commits` already checks that trailers name real artifacts; nothing checked the converse — that an artifact a merged commit claims to implement is no longer sitting at `proposed`/`draft`. The cost is that release readiness, defined as a query over `release:` plus `status`, reports shipped scope as unshipped. A release looks further away than it is and a human has to re-derive the truth from git log. That happened five times in one cycle, always caught by manual audit. Run against rivet's own history on landing it, the check reports 30 artifacts — DD-064, DD-066, FEAT-071, REQ-030, REQ-041..047 among them — that a commit claims to implement while they sit at `draft`. Only `implements`/`fixes` count. `verifies` and `traces-to` make no claim that the artifact is done, so advancing on those would be wrong. Deliberately does NOT auto-advance status. Whether shipped code discharges every acceptance clause is a judgement (REQ-308), and a tool that flipped `proposed` to `verified` because a trailer exists would manufacture exactly the false confidence that requirement is about. The check names the artifact and the commit; a human decides which status is right. Advisory by default, error under `--strict`: a genuinely partial implementation may legitimately carry a trailer while the artifact stays `proposed`, so drift alone must not fail an ordinary run, and CI can still enforce it. CI is unaffected. Its commits gate reads only the orphans and broken-refs counts out of `--format json` and does not pass `--strict`, so the default exit code is unchanged and `status_drift` is an additive field. Computed in the CLI rather than added to `CommitAnalysis`, whose fields are public — a new one there would be a breaking change and trip semver-checks for no benefit. The oracle failed for the wrong reason twice before it was honest: first the fixture had no `commits:` section, then its `REQ-SHIPPED` id is not the `PREFIX-<number>` shape a trailer parses, so nothing linked and the test exercised no code at all. It now asserts `Linked: 1` BEFORE looking for drift, and scopes the negative case to the new section — `REQ-002` legitimately appears elsewhere in the report, so a whole-output check would have been satisfied by the wrong text. Negative-controlled: neutering the status test reddens the oracle. Confirmed with cargo fmt --check, clippy --all-targets -D warnings on 1.97.0, cargo test --workspace (exit 0, 65 ok, 0 failed), the full cargo test -p rivet-cli --test cli_commands suite (187 passed, 0 failed), rivet validate, rivet docs check, yamllint on the CI paths — all exit 0. Implements: REQ-315 Verifies: REQ-315
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Rivet Criterion Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 1ddd605 | Previous: 6254d59 | Ratio |
|---|---|---|---|
store_insert/100 |
85862 ns/iter (± 525) |
67056 ns/iter (± 639) |
1.28 |
store_insert/10000 |
15534813 ns/iter (± 1305541) |
10432694 ns/iter (± 453622) |
1.49 |
store_lookup/100 |
2123 ns/iter (± 11) |
1316 ns/iter (± 23) |
1.61 |
store_lookup/1000 |
26184 ns/iter (± 247) |
15529 ns/iter (± 347) |
1.69 |
store_lookup/10000 |
381761 ns/iter (± 11553) |
239306 ns/iter (± 2178) |
1.60 |
store_by_type/100 |
95 ns/iter (± 0) |
67 ns/iter (± 0) |
1.42 |
store_by_type/1000 |
95 ns/iter (± 0) |
68 ns/iter (± 0) |
1.40 |
store_by_type/10000 |
95 ns/iter (± 0) |
68 ns/iter (± 3) |
1.40 |
schema_load_and_merge |
1519194 ns/iter (± 17139) |
1202535 ns/iter (± 12975) |
1.26 |
link_graph_build/100 |
166501 ns/iter (± 1183) |
138657 ns/iter (± 315) |
1.20 |
link_graph_build/1000 |
1947153 ns/iter (± 36732) |
1604888 ns/iter (± 7569) |
1.21 |
link_graph_build/10000 |
37963784 ns/iter (± 5762738) |
23263288 ns/iter (± 597344) |
1.63 |
validate/100 |
502525 ns/iter (± 5929) |
378838 ns/iter (± 7633) |
1.33 |
validate/1000 |
18490400 ns/iter (± 308750) |
13789929 ns/iter (± 707123) |
1.34 |
validate/10000 |
1515905343 ns/iter (± 14340156) |
1010801366 ns/iter (± 7820331) |
1.50 |
traceability_matrix/100 |
4320 ns/iter (± 36) |
3137 ns/iter (± 144) |
1.38 |
traceability_matrix/1000 |
61223 ns/iter (± 943) |
35063 ns/iter (± 381) |
1.75 |
diff/100 |
62321 ns/iter (± 235) |
47664 ns/iter (± 1563) |
1.31 |
diff/1000 |
709614 ns/iter (± 4758) |
491188 ns/iter (± 1951) |
1.44 |
diff/10000 |
9358777 ns/iter (± 814160) |
6125433 ns/iter (± 32230) |
1.53 |
query/100 |
1142 ns/iter (± 3) |
808 ns/iter (± 21) |
1.41 |
query/1000 |
14832 ns/iter (± 113) |
10314 ns/iter (± 50) |
1.44 |
document_parse/10 |
22895 ns/iter (± 169) |
18078 ns/iter (± 57) |
1.27 |
document_parse/100 |
158451 ns/iter (± 712) |
125632 ns/iter (± 9433) |
1.26 |
document_parse/1000 |
1480103 ns/iter (± 74049) |
1159888 ns/iter (± 65984) |
1.28 |
This comment was automatically generated by workflow using github-action-benchmark.
📐 Rivet artifact delta
Graphgraph LR
REQ_315["REQ-315"]:::modified
classDef added fill:#d4edda,stroke:#28a745,color:#155724
classDef removed fill:#f8d7da,stroke:#dc3545,color:#721c24
classDef modified fill:#fff3cd,stroke:#ffc107,color:#856404
classDef overflow fill:#e2e3e5,stroke:#6c757d,color:#495057,stroke-dasharray: 3 3
Modified
Posted by |
Codecov Report
✅ All modified and coverable lines are covered by tests.
📢 Thoughts on this report? Let us know!
avrabe
added a commit
that referenced
this pull request
Sep 5, 2026
...EQ-316) Two CI signals were sending readers to the wrong conclusion, and both had already produced a published misattribution. REQ-317 — the liveness probe had three defects, all fixed: 1. Its diagnose hint told every reader to run `gh api repos/<repo>/actions/runners --jq .total_count` and read `0 = pool offline`. Runners here are ORG-registered, so that command returns 0 on a perfectly healthy pool and can never say otherwise. The alert's own hint was the trap. 2. It reported a duration and nothing else, so hosted starvation and a dead self-hosted pool produced identical alerts — and hosted capacity appears in the runners API at NO scope, so no runner count can tell them apart. 3. It counted queue age on runs that had already COMPLETED: #867 reported "queued 177m" for a cancelled run that was never going to start. REQ-316 — a job killed by a fleet restart reports `failure` with NO failed step, because the steps end `skipped`. GitHub renders that identically to a broken test, which is how a wrong cause got published on #855. Both are now shell functions in tools/ci-diagnose/diagnose.sh with fixtures: classify_stall no-queue | hosted-starved | pool-offline | label-saturated | capacity-available classify_failure job-passed | runner-vanished | setup-failed | job-failed A FIFTH failure mode turned up while building this and is covered: label- partitioned saturation. On 2026年09月02日 the pool read online=12 busy=8 — a third of the fleet idle — while every rust-cpu job queued, because the four idle runners carried only lean-mem. A global busy-versus-online count cannot see that, so capability is computed PER LABEL against the labels of what is queued. Validated against live data, not only fixtures: with 5 hosted and 3 rust-cpu jobs queued and only lean-mem and light runners idle, classify_stall returns label-saturated; the control that filters to just the hosted jobs returns hosted-starved. classify_failure on the real Kani job from #888 returns setup-failed — the same discriminator applied by hand all cycle, now mechanical. Negative-controlled: ignoring per-label capability, losing the runner-vanished case, and dropping the hosted short-circuit each redden the oracle. The tests run in the YAML Lint job, which IS in CI Gate's needs. A classifier that never executes is documentation, not a gate. They are shell, and their `rivet: verifies` markers are picked up by `coverage --tests` only because REQ-319 taught the scanner to read shell scripts earlier in this same round — before that they would have been invisible. REQ-316 is SPLIT, decided with the maintainer. This carries the legibility half. Finding the cause on the runner hosts — unattended upgrades, a systemd timer, config management or an autoscaler reclaim, all consistent with the 04:30-05:00 UTC window both incidents fall in — needs shell access to those machines and is deferred to REQ-323 at v0.37.0. Recorded, not dropped. Confirmed with the diagnose oracle (10 cases), cargo fmt --check, cargo test --workspace (exit 0, 66 ok), rivet validate, rivet docs check, yamllint on the CI paths — all exit 0. Both workflow files parse as YAML and every embedded shell block passes `bash -n`. Implements: REQ-316, REQ-317 Verifies: REQ-316, REQ-317
avrabe
added a commit
that referenced
this pull request
Sep 5, 2026
...EQ-316) Two CI signals were sending readers to the wrong conclusion, and both had already produced a published misattribution. REQ-317 — the liveness probe had three defects, all fixed: 1. Its diagnose hint told every reader to run `gh api repos/<repo>/actions/runners --jq .total_count` and read `0 = pool offline`. Runners here are ORG-registered, so that command returns 0 on a perfectly healthy pool and can never say otherwise. The alert's own hint was the trap. 2. It reported a duration and nothing else, so hosted starvation and a dead self-hosted pool produced identical alerts — and hosted capacity appears in the runners API at NO scope, so no runner count can tell them apart. 3. It counted queue age on runs that had already COMPLETED: #867 reported "queued 177m" for a cancelled run that was never going to start. REQ-316 — a job killed by a fleet restart reports `failure` with NO failed step, because the steps end `skipped`. GitHub renders that identically to a broken test, which is how a wrong cause got published on #855. Both are now shell functions in tools/ci-diagnose/diagnose.sh with fixtures: classify_stall no-queue | hosted-starved | pool-offline | label-saturated | capacity-available classify_failure job-passed | runner-vanished | setup-failed | job-failed A FIFTH failure mode turned up while building this and is covered: label- partitioned saturation. On 2026年09月02日 the pool read online=12 busy=8 — a third of the fleet idle — while every rust-cpu job queued, because the four idle runners carried only lean-mem. A global busy-versus-online count cannot see that, so capability is computed PER LABEL against the labels of what is queued. Validated against live data, not only fixtures: with 5 hosted and 3 rust-cpu jobs queued and only lean-mem and light runners idle, classify_stall returns label-saturated; the control that filters to just the hosted jobs returns hosted-starved. classify_failure on the real Kani job from #888 returns setup-failed — the same discriminator applied by hand all cycle, now mechanical. Negative-controlled: ignoring per-label capability, losing the runner-vanished case, and dropping the hosted short-circuit each redden the oracle. The tests run in the YAML Lint job, which IS in CI Gate's needs. A classifier that never executes is documentation, not a gate. They are shell, and their `rivet: verifies` markers are picked up by `coverage --tests` only because REQ-319 taught the scanner to read shell scripts earlier in this same round — before that they would have been invisible. REQ-316 is SPLIT, decided with the maintainer. This carries the legibility half. Finding the cause on the runner hosts — unattended upgrades, a systemd timer, config management or an autoscaler reclaim, all consistent with the 04:30-05:00 UTC window both incidents fall in — needs shell access to those machines and is deferred to REQ-323 at v0.37.0. Recorded, not dropped. Confirmed with the diagnose oracle (10 cases), cargo fmt --check, cargo test --workspace (exit 0, 66 ok), rivet validate, rivet docs check, yamllint on the CI paths — all exit 0. Both workflow files parse as YAML and every embedded shell block passes `bash -n`. Implements: REQ-316, REQ-317 Verifies: REQ-316, REQ-317
avrabe
added a commit
that referenced
this pull request
Sep 5, 2026
...EQ-316) (#890) Two CI signals were sending readers to the wrong conclusion, and both had already produced a published misattribution. REQ-317 — three defects in the liveness probe, all fixed. Its diagnose hint told every reader to run a REPO-scope actions/runners total_count query and read 0 = pool offline; runners here are ORG-registered, so that returns 0 on a healthy pool and can never say otherwise — the alert's own hint was the trap. It reported a duration and nothing else, so hosted starvation and a dead self-hosted pool produced identical alerts, and hosted capacity appears in the runners API at NO scope. And it counted queue age on runs that had already completed: #867 reported "queued 177m" for a cancelled run. REQ-316 — a job killed by a fleet restart reports failure with NO failed step, because the steps end skipped. GitHub renders that identically to a broken test, which is how a wrong cause got published on #855. Both are now shell classifiers with fixtures. A fifth failure mode turned up while building this and is covered: label-partitioned saturation, measured 2026年09月02日 as online=12 busy=8 while every rust-cpu job queued, because the four idle runners carried only lean-mem. Capability is computed PER LABEL against the labels of what is queued. Validated against live data, not only fixtures: 5 hosted and 3 rust-cpu jobs queued with only lean-mem and light idle returns label-saturated, the control filtering to just the hosted jobs returns hosted-starved, and the real Kani job from #888 returns setup-failed. That last classification was then used to re-derive #839's failure rate: all 26 Kani failures across 60 jobs are the install action, none is a proof break, and none falls in the needrestart window. The tests run in the YAML Lint job, which IS in CI Gate's needs — a classifier that never executes is documentation, not a gate. Their markers are read by coverage --tests only because REQ-319 taught the scanner to read shell scripts earlier in this same round. REQ-316 is SPLIT with the maintainer: this carries the legibility half. The root cause was found on #862 (needrestart restarting github-runner@ under unattended-upgrades) and is recorded on REQ-323 as implemented-not-verified, since suppression of a real restart has not been demonstrated yet. Implements: REQ-316, REQ-317 Verifies: REQ-316, REQ-317 Refs: REQ-323, REQ-319
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A trailer is a link, not a state transition.
rivet commitsalready checksthat trailers name real artifacts; nothing checked the converse — that an
artifact a merged commit claims to implement is no longer sitting at
proposed/draft.The cost is that release readiness, defined as a query over
release:+status, reports shipped scope as unshipped. A release looks further awaythan it is and a human has to re-derive the truth from
git log. That happenedfive times in one cycle, each time caught only by manual audit.
Run against rivet's own history, the check immediately reports 30 artifacts
a commit claims to implement while they sit at
draft:Scope decisions
implements/fixescount.verifiesandtraces-tomake no claimthe artifact is done.
clause is a judgement (REQ-308). A tool that flipped
proposed→verifiedbecause a trailer exists would manufacture exactly the false confidence that
requirement is about. It names the artifact and the commit; a human decides.
--strict. A genuinely partialimplementation may legitimately carry a trailer, so drift alone must not fail
an ordinary run — but CI can enforce it.
Blast radius
CI is unaffected. Its commits gate reads only the
orphansandbroken_refscounts out of--format jsonand does not pass--strict, so thedefault exit code is unchanged and
status_driftis an additive field.Computed in the CLI rather than added to
CommitAnalysis, whose fields arepublic — a new field there would be a breaking change and trip semver-checks for
no benefit.
The oracle failed for the wrong reason twice
Worth stating, since it nearly produced a fix for a broken fixture:
commits:section — the command errored before checking;REQ-SHIPPEDis not thePREFIX-<number>shape a trailer parses, so nothinglinked and the test exercised no code at all.
It now asserts
Linked: 1before looking for drift, and scopes the negativecase to the new section —
REQ-002legitimately appears elsewhere in thereport, so a whole-output check would have been satisfied by the wrong text.
Negative-controlled: neutering the status test reddens the oracle.
Gates
cargo fmt --check·clippy --all-targets -D warningson 1.97.0 ·cargo test --workspace(exit 0, 65 ok) · fullcli_commands(187 passed) ·rivet validate·rivet docs check·yamllint— all exit 0.Second of the v0.36.0 feature-loop run.