feat: real drift probes for the five remaining documentation-only plugins - #320
Merged
Merged
Conversation
Contributor
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Documentation | 71 minor |
| Security | 29 high |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
3 tasks
enriquea
force-pushed
the
feat/real-drift-probes-remaining-stubs
branch
from
September 1, 2026 07:47
fe8419a to
4e4adb2
Compare
…gins Completes the sweep #319 began. Issue #177 deferred all seven stubs after validating that an hgnc-style probe was infeasible. Re-checking each source against what it actually serves, none of the five verdicts held as stated -- though for three of them the *data* really is unreachable, and the probes here say so rather than implying otherwise. Direct file probes (the data itself is addressable): * gnomad-metrics -- the constraint tables sit in the public gcp-public-data bucket and return an MD5 ETag, a Content-Length and x-goog-generation. #177 rated this "marginally feasible"; it is in fact stronger than the hgnc reference, since GCS ETags are content digests rather than mtime validators. All three declared tables are probed, so the fingerprint covers whichever release a build pinned. Release-stream probes (the data is gated or gone; the release is not): * dbnsfp -- every dbNSFP*.zip the landing page advertises is dead (the S3 bucket answers NoSuchBucket) and the liulab mirror does not resolve, so #177 was right that no data URL works. The page still lists the release set, so a roll-over is detectable. * cosmic-cgc -- /census 302s to the login form, but /cosmic/release_notes is served publicly and names the current release. Note the trailing slash matters: the path without it returns 200, with it redirects to login. * pqtl -- the statistics ship as supplementary material to a medRxiv preprint, so the publication is the upstream. medRxiv's public JSON API exposes the version and whether a journal version now exists. * alphagenome -- a credentialed prediction service has no static artifact, but what makes a stored artifact stale is the service moving, and the SDK release stream is published openly on PyPI. Each of the four release-stream probes documents what it cannot see (an in-place change under an unchanged release/version), so the coverage claim stays honest rather than implying file-level watching. Two design points, both empirical: * Fingerprint the projection, never the page. Two consecutive fetches of the dbNSFP landing page returned 352,830 and 352,716 bytes, so hashing the body would have flagged drift on every run and opened a nightly no-op PR. The extracted release list was byte-stable across the same fetches. COSMIC's page happens to be stable, but is treated the same way so an unrelated editorial edit cannot trigger a rebuild. * Exclude editorial fields. pqtl compares version/date/published/doi and not the title or abstract, so a typo fix cannot open a pull request -- the same correction hgnc needed for Last-Modified. Also fixes a real bug caught by its own test: requests' JSONDecodeError subclasses both ValueError and RequestException, so decoding inside the request try-block reported a malformed body as an HTTP failure. The pqtl and alphagenome probes now parse in a separate block. dbnsfp and gnomad-metrics leave KNOWN_INCOMPLETE entirely; the other three shrink to their remaining snapshot gaps. Datasets with a committed fingerprint: 21 -> 26 of 26. Datasets with a live comparator: 18 -> 23 of 26 (26 once #319 lands, which clears the last three stubs, gevir and both insider datasets). Refs #177
…obes Rebased onto the gevir/insider branch so this work inherits the two core fixes that review produced: `normalize_etag` and the offline-build degradation. #319 must therefore merge first -- the PR body's claim that merge order does not matter was wrong, because without the run_builder fix these five probes make `hvantk reprocess` require network on exactly the manual-acquisition plugins that are built from hand-staged files on nodes with no egress. **Two probes were reading the wrong thing entirely.** dbNSFP matched `dbNSFP(\d...)` , which requires a digit straight after the name and so silently missed every `v`-prefixed archive -- the whole 2.x and 3.x generation. The committed baseline held 30 releases; it should have held 59. A next release named `dbNSFPv5.0a.zip` would have left the projection unchanged and reported clean on a real roll-over, which is the false-green the stub existed to prevent. COSMIC matched `COSMIC v<N>` anywhere in the body and produced [v16, v18, v20, v101..v104] -- a non-contiguous set that is not a release index. v16/v18/v20 come from prose about the *Actionability* product, a different version series ("COSMIC v20 of the Actionability data"). Any editorial sentence naming an old release would have opened a no-op PR, and a "coming in v105" would have reported a release that did not exist. The page carries `id="v101"`... anchors, one per real release; the probe now reads those and gets a clean contiguous index. **The fingerprint shape was inverted in all five.** The content signal sat in `checksums`, which `_conventions` § 12 defines as "sha256 over the bytes used to derive `headers`" -- these probes fetch no body and compute no such digest, and gnomad_metrics was storing a raw ETag there. Signals moved under `headers` with `checksums` empty, matching peptideatlas and the sibling branch. The redundant sha256 went with them: it was a pure function of values already in the compared surface, so it added no detection power while doubling every diff. Note this keeps changes tiered "schema" rather than "routine". That is deliberate, per `classify_risk`'s own policy -- "Defaults to schema for anything it cannot read ... the reverse just opens one extra PR" -- since none of these probes can see column names. **gnomad_metrics carried the sibling branch's bugs.** `.strip('"')` (mangles weak and transform-suffixed validators) replaced with `normalize_etag`; the `and` guard that accepted a half-validated response -- silently dropping an object out of the compared surface, or baking a null length in -- now requires both and rejects empties; a non-identity `Content-Encoding` response is refused rather than recording a compressed length. Three sequential HEADs at 30s each had a 180s worst case against the runner's 60s SIGALRM, so a slow bucket reported probe_failed on a healthy source; now a (5, 10) connect/read pair on one shared Session. Failures are collected across objects instead of raising on the first, which had let a fault on either frozen v2.1.1 table abort before v4.0 -- the current release -- was probed at all. **pqtl and alphagenome rested on undocumented upstream shapes.** `collection[-1]` assumed medRxiv orders oldest-first; on a newest-first response it pinned v1 and would have reported clean through the exact event the probe exists to detect. Now selected by version. `str(latest.get("version"))` turned a missing field into the literal "None" -- truthy, and indistinguishable from a real label; now fails closed. `doi` left the compared surface: the request URL is built from it, so it could never drift. alphagenome no longer hard-requires PyPI's deprecated `releases` key, which would have turned an upstream API change into a permanent probe_failed for an SDK that never moved; `info.version` is the signal, and the release list is informational so pre-release uploads cannot fire a no-op PR. **All five now use `request_with_retry`**, the helper written after a single GenCC 429 failed the 2026-08-04 scheduled run. Bare `requests` had reintroduced that bug five times, against two hosts (PyPI, api.biorxiv.org) that rate-limit unauthenticated clients. Both HTML scrapes also decode explicitly rather than letting requests guess a charset, and COSMIC rejects a redirected response instead of scraping a login page that answers 200. **Claims I made that were not true, now made true or removed.** Four docstrings asserted their coverage limits were "recorded in SKILL.md" while all five SKILL.md files still said the probe was a stub -- CONTRIBUTING.md:26 rejects exactly this. Every one now documents what its probe detects AND what it cannot, and cosmic_cgc gained the drift section it never had. The dbNSFP docstring cited "31 entries" against a baseline holding 30. The KNOWN_INCOMPLETE comment said "the remaining three entries" of a five-entry dict and described them as having nothing to probe, when this PR gave all three a live probe. **Tests that could not fail, rewritten.** The gnomad fixtures gave all three objects identical headers and only exercised the both-missing guard; there are now distinct per-object fixtures and a parametrized case per degradation. The dbNSFP "ordering" test never varied order; the COSMIC "numeric sort" test could not fail against an int-typed implementation; alphagenome's guard test tripped both branches at once. Added a new suite validating every committed baseline at rest -- `placeholder_baseline_reason`, a real `fetched_at`, and a non-empty compared surface -- which is the check the ledger's "captured from live probe runs" claim leaned on and nothing enforced. The dbNSFP dead-download-links finding is tracked as #321 and referenced from the probe and SKILL.md rather than left only in a Python comment. Refs #177, #321
enriquea
force-pushed
the
feat/real-drift-probes-remaining-stubs
branch
from
September 1, 2026 07:57
4e4adb2 to
c87342e
Compare
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
2 participants
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.
Completes the sweep #319 began. Together they close #177.
Why the original deferral no longer holds
#177 concluded these sources had "no probeable URL" and shipped stub sentinels. Re-checking each against what it actually serves, none of the five verdicts held as stated -- though for four of them the data really is unreachable, and the probes say so rather than implying otherwise.
One direct file probe.
gnomad_metricswas rated "marginally feasible (fragile)". Its constraint tables sit in the publicgcp-public-data--gnomadbucket and return an MD5 ETag plus Content-Length andx-goog-generation-- a stronger comparator than the hgnc reference, since GCS ETags are content digests rather than mtime-derived validators.Four release-stream probes, where the data is gated or gone but the release is public:
dbnsfpdbNSFP*.zip404s (NoSuchBucket); mirror does not resolvecosmic_cgc/census302s to login/cosmic/release_notes, served publiclypqtlalphagenomeEach documents what it cannot see, in its docstring and its SKILL.md.
What the adversarial review changed
Two probes were reading the wrong thing. dbNSFP's pattern required a digit straight after the name, so it missed every
v-prefixed archive -- the whole 2.x/3.x generation. The baseline held 30 releases; it should have held 59, and a next release nameddbNSFPv5.0a.zipwould have reported clean on a real roll-over. COSMIC matched prose and produced[v16, v18, v20, v101..v104]-- v16/v18/v20 come from sentences about the Actionability product, a different version series. It now reads the page'sid="v<N>"anchors and gets a clean contiguous index.The fingerprint shape was inverted in all five. The content signal sat in
checksums, which_conventions§12 defines as a sha256 over the bytes used to deriveheaders-- these probes fetch no body, andgnomad_metricswas storing a raw ETag there. Signals moved underheaders,checksumsleft empty, and the redundant sha256 dropped (it was a pure function of values already compared). Changes stay tiered "schema", deliberately, perclassify_risk's own stated policy for probes that cannot read columns.gnomad_metricscarried the sibling branch's bugs:.strip('"')tonormalize_etag; theandguard that accepted a half-validated response now requires both validators and rejects empties; a non-identityContent-Encodingis refused; 3x30s sequential HEADs (180s worst case against a 60s SIGALRM) became a(5, 10)pair on one shared Session; and failures are collected across objects rather than aborting before v4.0 is ever probed.pqtl/alphagenomerested on undocumented shapes:collection[-1]assumed medRxiv orders oldest-first and would have missed the one event the probe exists to detect;str(None)recorded a literal"None"as a version;doiwas a constant in the compared surface; and alphagenome no longer hard-requires PyPI's deprecatedreleaseskey.All five now use
request_with_retry-- the helper written after a single GenCC 429 failed the 2026-08-04 run. Both HTML scrapes decode explicitly instead of letting requests guess a charset, and COSMIC rejects a redirected response rather than scraping a login page that answers 200.Claims that were not true, now fixed. Four docstrings said their limits were "recorded in SKILL.md" while all five SKILL.md files still called the probe a stub (CONTRIBUTING.md:26 rejects exactly this). The dbNSFP docstring cited 31 entries against a 30-entry baseline. The
KNOWN_INCOMPLETEcomment said "the remaining three entries" of a five-entry dict.Tests that could not fail, rewritten -- identical per-object fixtures, an "ordering" test that never varied order, a "numeric sort" test that could not fail against an int-typed implementation. Plus a new suite validating every committed baseline at rest (
placeholder_baseline_reason, a realfetched_at, a non-empty compared surface) -- the check the ledger's "captured from live probe runs" claim leaned on and nothing enforced.Verification
hvantk driftreturnscleanfor all eight datasets across both PRs.ruff checkclean on every file touched.Effect
Committed fingerprints 21 to 26 of 26; live comparators 18 to 26 of 26 with #319. No stub sentinels remain.
dbNSFP's dead download links are tracked separately as #321 -- a documentation defect, not a drift signal.