ci: feed merged unit+integration coverage into the fallow structural quality gate - #909
Merged
Merged
Conversation
…quality gate The fallow audit previously ran in its own workflow (fallow.yml) against a unit-only coverage pass it generated itself, so functions covered only by the integration suite (the testing-trophy convention for services/routes) scored 0% and produced false CRAP-inflation findings (PR #908: 5 findings on functions with 11 dedicated integration tests). The integration test step in build.yaml's `test` job now records its own coverage (instrumenting the existing run, not a second one) into coverage-integration/, separate from the unit ratchet's coverage/. Both are merged (nyc merge) after sanitizing negative hit counts that vitest's v8-coverage provider can emit under the integration project's multi-fork pool — an existing artefact, discovered while testing this change, that otherwise hard-errors fallow's coverage parser. The merged file is uploaded as an artifact and consumed by a new `structural-quality` job (required check name "Structural Quality (blocking)", preserved) that runs the same fallow invocation fallow.yml used to, verbatim in semantics, including the PR-comment posting and fail-closed exit-code handling. fallow.yml is retired (stripped to a no-op with no trigger, kept for the retirement note in its history — .github/ deletions are blocked here). .fallowrc.json now ignores the new coverage-* directories, which fallow's whole-tree discovery was otherwise scanning as source files. The structural-quality job branches explicitly on needs.test.result rather than relying on either skip-propagation reading or an implicit success() check (see its header comment for the both-readings analysis), so a failed or cancelled test job fails the gate instead of silently reporting green via skip, while a docs-only PR (test genuinely skipped) reports a clean success with nothing to audit. Baseline refresh recipe for .fallow-baseline.health.json under the new merged-coverage input is documented in build.yaml's header comment; Chris/cid runs and commits it separately.
…nt-only) Nanaki PASS-WITH-NOTES / vincent APPROVE-WITH-NOTES on 0acb949. Four must-fixes, all comment-tier: 1. The structural-quality header comment claimed cancellation was handled fail-closed. It isn't: GitHub force-skips a not-yet-started job on whole-run cancellation before its `if:` is evaluated, so `!cancelled()` never runs and a skipped required check passes branch protection — a regression from the old standalone fallow.yml job, which reported its own 'cancelled' conclusion and blocked merge. Rewrote both the structural-quality comment and the fallow.yml retirement note to state this accurately instead of claiming fail-closed. 2. Added an explicit accepted-risk line next to the corrected comment: documented GitHub Actions limitation, no in-workflow fix; compensating control is process (a cancelled run must be re-run to completion before merge); a stronger technical fix is out of scope, separate issue if it bites in practice. 3. fallow.yml's `on: workflow_dispatch: {}` contradicted its own "no trigger, never runs" claim. Verified with actionlint that both an empty `on: {}` and a missing `on:` key are hard errors ("on section should not be empty" / "on section is missing") — a genuinely trigger-less workflow isn't valid, so workflow_dispatch stays. Corrected the comment to say it exists only to keep the file schema-valid, and that a manual run is harmless (different job name, doesn't satisfy or collide with the required check). 4. Could not reproduce the .fallowrc.json causal claim under nanaki's method either (774 total_files with or without the three new ignorePatterns entries, tested directly against this worktree's own coverage-integration/ HTML report tree). The earlier "discovered 1130 files" observation was from a stray duplicate scratch directory, not from the ignorePatterns state. JSON has no comment syntax so there is no inline claim to soften in .fallowrc.json itself; the correction belongs in the PR description (commit 0acb949's message stands per Chris/cid's call) — restating the patterns as a defensive addition, not an observed-problem fix. No step logic, `if:` conditions, flags, or job structure changed — diffed non-comment/non-blank lines against 0acb949 to confirm.
Fallow audit reportNo GitHub PR/MR findings. Generated by fallow. |
chrisdburr
added a commit
that referenced
this pull request
Aug 25, 2026
…verage First baseline saved under the merged-coverage recipe that PR #909's gate now audits with (identity mode, real unit+integration coverage, negative counts sanitized per the build.yaml merge step). The previous baseline was saved with unit-only coverage, which inflated CRAP scores on integration- tested functions; under the merged recipe those findings cease to exist, hence the large shrink. A baseline saved under the old recipe would not pair with merged-coverage head scores (fallow-rs/fallow#2347 class).
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
1 participant
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.
What
Root-cause fix for the Structural Quality gate's coverage blind spot. The gate previously audited with unit-test coverage only, so service/route functions covered exclusively by the integration suite (the repo's testing-trophy convention) scored 0% and failed on inflated CRAP scores — most recently five false positives on PR #908.
Now: the existing integration-test run in build.yaml also records coverage (~0-2.6% duration overhead), unit + integration coverage are merged (fail-closed if either input is missing), and the fallow audit runs on the merged file in a
structural-qualityjob that keeps the exact required-check name "Structural Quality (blocking)". The standalone fallow.yml workflow is retired to a no-op stub (deletion is hook-blocked; itsworkflow_dispatchtrigger exists only because a trigger-less workflow is schema-invalid, and its job name cannot satisfy the required check).Verification highlight: with real merged coverage, the PR #908 false positives disappear while a genuinely-uncovered function in the same file correctly stays flagged — the gate discriminates real gaps from estimation artefacts.
Notes
Review chain
Unblocks #908.