Suppress unactionable PRs, and fix the cache bug that made the trigger filter inert - #11
Merged
Merged
Conversation
The ETag cache stores projections, not raw responses, so its contents are only meaningful to the code that wrote them. Nothing enforced that. Adding the workflow trigger list changed the cached workflow definition from a bare `Vec<String>` of crons to a struct. Every persisted entry kept the old shape across the upgrade while its ETag stayed valid, so GitHub answered 304 and the 304 path returned a decode error. The caller treats a failed definition lookup as "triggers unknown", and an empty trigger list means "accept any event" -- so the trigger filter was inert in production for as long as the cache file survived. That is not a cosmetic bug: it is the exact frext / bike-fitter-1000 false positive the filter was written to remove, still being reported weeks after the fix shipped. Confirmed on sdrhub, where every cached contents entry was still in the pre-upgrade format. Two independent mechanisms, because one of them already failed: * The cache file carries a format version. A mismatch discards the file wholesale, since a projection change invalidates all of it, at a cost of one cold sweep. * An undecodable entry is a cache miss rather than an error. The stale ETag is dropped and the resource refetched unconditionally. This is what recovers from a forgotten version bump -- the failure mode that actually happened -- and it is why the fix does not depend on remembering to bump anything. Verified against the live API with a pre-versioning cache file seeded on disk: it is rejected, and frext and bike-fitter-1000 no longer report their superseded push failures.
Some pull requests are stuck and will stay stuck. #32 on
sdr-enthusiasts/docker-vesselalert is open against a repository we do not
own, conflicting, and the maintainer has not engaged with it. It is not
ours to close or convert to a draft, so it alerts forever.
Nothing the exporter can measure separates that from a pull request worth
chasing, so it has to be declared:
ignore_pulls = ["sdr-enthusiasts/docker-vesselalert#32"]
Suppression is scoped to actionability, not visibility. An ignored PR is
dropped from the per-PR series -- needs_attention, ready_to_merge, and
created_timestamp -- so no alert can fire on it and it leaves the
dashboard's PR tables. It still counts towards repo_pulls_open, because
the repository really does have it open and that count must keep matching
GitHub's own. The number suppressed is published per repository as
repo_pulls_ignored, including as a zero, so "nothing is hidden here" is
an assertion the dashboard can make rather than an absence it has to
assume.
Deliberately per-PR rather than reusing `denylist`: denylisting the
repository would also blind the exporter to its CI state and to every
future pull request on it.
Entries are parsed into a PullRef at startup, so a typo is a startup
error rather than a filter that silently never matches -- the worst
outcome being an operator who believes a PR is suppressed while its alert
keeps firing. Matching is case-insensitive, since the entry is
hand-written and the runtime value comes from the API.
Verified against the live API: #32 is absent from every per-PR series
while repo_pulls_open still reports 1 and repo_pulls_ignored reports 1,
and the other 47 monitored repositories are unaffected.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe exporter adds per-pull-request suppression through configuration and NixOS wiring. It reports ignored pull counts while preserving aggregate open-pull totals. The GitHub client now uses versioned cache files and refetches when cached projections cannot be decoded. ChangesPull-request suppression
Cache versioning and recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Config
participant Collector
participant Metrics
Config->>Collector: provide ignored PullRef set
Collector->>Metrics: omit matching per-pull metrics
Collector->>Metrics: publish repository ignored count
sequenceDiagram
participant GithubClient
participant Cache
participant GitHubAPI
GithubClient->>GitHubAPI: send conditional request with ETag
GitHubAPI-->>GithubClient: return 304 response
GithubClient->>Cache: decode cached projection
Cache-->>GithubClient: return decode failure
GithubClient->>GitHubAPI: refetch without stale ETag
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@github-ci-exporter/src/github/client.rs`:
- Around line 255-267: Update with_cache_file to handle Err from
std::fs::read_to_string explicitly: log non-NotFound read failures with warn!,
while continuing silently for a missing cache file and preserving the existing
decode and cache-restore behavior.
- Around line 492-510: Update the stale-projection recovery flow around the
cached response match to remove the stale cache entry, clear cached_etag, and
continue through the existing request loop rather than issuing a direct
unconditional request. Preserve classify_retry handling so recovery responses
such as 429 and 5xx receive the normal bounded retries.
In `@github-ci-exporter/src/model.rs`:
- Around line 220-222: Update the is_full_name validation in PullRef::new to
reject repository strings containing whitespace, including owner/name `#32`,
before retaining the value. Extend the malformed-input tests with
whitespace-containing repository values and verify they are rejected.
In `@README.md`:
- Around line 81-83: Update the README guidance to reference
CACHE_FORMAT_VERSION using the repository-relative path
github-ci-exporter/src/github/client.rs, or explicitly state that
github/client.rs is relative to github-ci-exporter/src.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 78252087-a1d4-42e0-a98a-ab58fa5a3012
📒 Files selected for processing (7)
README.mdgithub-ci-exporter/src/collector.rsgithub-ci-exporter/src/config.rsgithub-ci-exporter/src/github/client.rsgithub-ci-exporter/src/metrics.rsgithub-ci-exporter/src/model.rsnix/nixos-module.nix
There was a problem hiding this comment.
Pull request overview
This PR (1) hardens the GitHub API ETag cache so cached projection shape changes cannot silently disable workflow-trigger filtering, and (2) adds an operator-configurable ignore_pulls mechanism to suppress unactionable PRs from per-PR alerting series while keeping aggregate counts honest.
Changes:
- Add a versioned on-disk envelope for the ETag cache and treat undecodable cached projections as cache misses (refetch) instead of hard errors.
- Introduce
ignore_pullsconfiguration (including NixOS module support) and a newgithub_repo_pulls_ignoredmetric; suppressed PRs are omitted from per-PR series but still counted ingithub_repo_pulls_open. - Extend documentation and tests to cover the new cache behavior and PR suppression logic.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents cache-format versioning/behavior and the new ignore_pulls suppression semantics and metric. |
| nix/nixos-module.nix | Adds NixOS option services.github-ci-exporter.ignorePulls and maps it to ignore_pulls. |
| github-ci-exporter/src/model.rs | Introduces PullRef (validated owner/name#number) with parsing + tests for case-insensitive matching. |
| github-ci-exporter/src/metrics.rs | Adds and registers repo_pulls_ignored metric family, and clears it between cycles. |
| github-ci-exporter/src/github/client.rs | Adds cache file versioning + decode logic, refetch-on-decode-failure for 304s, and tests for both behaviors. |
| github-ci-exporter/src/config.rs | Adds ignore_pulls config field, validates entries at load, and exposes parsed HashSet<PullRef>. |
| github-ci-exporter/src/collector.rs | Applies ignore list during activity recording and publishes github_repo_pulls_ignored per repo; adds tests for suppression behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+300
to
+303
| let encoded = serde_json::to_string(&CacheFile { | ||
| version: CACHE_FORMAT_VERSION, | ||
| entries: cache.clone(), | ||
| })?; |
Comment on lines
+164
to
+167
| // Validated at startup, so a parse failure here cannot happen; an empty | ||
| // set (ignore nothing) is the safe reading if it somehow does. | ||
| let ignored_pulls = config.ignored_pulls().unwrap_or_default(); | ||
|
|
Six findings from the PR review, two of which were real bugs: `PullRef` accepted internal whitespace. Only the outer edges were trimmed, so `owner/name #32` parsed successfully and then never matched an API-supplied repository name -- recreating the exact silent no-op the validation exists to prevent. Whitespace anywhere in the repository portion is now rejected, with the cases added to the malformed-input test. The stale-projection recovery request bypassed the retry loop. It was issued inline, outside `classify_retry`, so a 429 or 5xx on the refetch failed the cycle instead of backing off. It now clears the validator and re-enters the existing loop, which also deletes the duplicated request/ETag/status handling. That introduces one new way to spin -- a server answering 304 to a request carrying no If-None-Match -- so that is now an explicit fast failure rather than another cache miss. The rest are hygiene: * `ignore_pulls` was re-parsed every cycle despite being validated once at startup. It is now resolved on first use and memoised in WorkflowCache, and passed by reference rather than cloned. * A non-NotFound error reading the cache file was silently swallowed. A permission or I/O failure otherwise shows up only as a persistently high request count. * `persist_cache` cloned the whole cache to serialise it. A borrowed write-side envelope produces identical JSON without the copy. * The README pointed at `github/client.rs`, which does not resolve from the repository root. Re-verified against the live API, including a warm restart: the versioned cache is accepted without a cold-start warning, and #32 stays suppressed while repo_pulls_open still reports it.
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.
Two changes, one commit each. They are related only in that the second is
why one of the three reported CI failures was bogus.
fix:the ETag cache could silently disable a filterThe trigger filter added in #9 was never active in production. The cache
stores projections rather than raw responses, so when
WorkflowDefinitionchanged from a bare
Vec<String>of crons to a struct, every persisted entrykept the old shape while its ETag stayed valid. GitHub answered
304, the 304path failed to decode the cached body, and the caller treats a failed
definition lookup as "triggers unknown" -- which means accept any event.
So
frextandbike-fitter-1000kept reporting months-oldpushfailures fora
ci.ymlthat declares onlypull_request, which is exactly what #9 set outto fix. Confirmed on sdrhub: every cached contents entry was still
pre-upgrade (
[]/["0 0 1 * *"]instead of the struct).Two independent mechanisms, because one of them already failed once:
recovers from a forgotten version bump, so the fix does not depend on anyone
remembering to bump it
feat:ignore_pullsFor a PR that is genuinely stuck and genuinely not ours:
#32onsdr-enthusiasts/docker-vesselalertis conflicting, open against a repo we donot own, and the maintainer has not engaged. Not ours to close or draft, so it
alerts forever.
Suppression is scoped to actionability, not visibility. An ignored PR
leaves the per-PR series so nothing can alert on it, but still counts in
repo_pulls_open-- the repo really does have it open -- and the countsuppressed is published as
repo_pulls_ignored, including a zero, so thedashboard can assert "nothing is hidden here" instead of assuming it.
Per-PR rather than reusing
denylist, which would also blind the exporter tothe repo's CI and to every future PR on it. Entries are parsed at startup, so a
typo is a startup error rather than a filter that silently never matches.
Verification
cargo xtask cigreen (118 tests, clippy, deny, machete). Both changes werealso run against the live API:
frext/bike-fitter-1000no longer report their supersededpushfailures#32is absent from every per-PR series whilerepo_pulls_openstill reports1andrepo_pulls_ignoredreports1; the other 47 monitored repos areunaffected. The configured entry was deliberately mixed-case, so this also
exercises case-insensitive matching.
Summary by CodeRabbit
New Features
owner/name#numberidentifiers.Bug Fixes
Documentation