promote: dev → main (security hardening, public Kestrel default, resolver/annotator/certificate) - #86
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…efMet with review flag Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…on sites Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CI runs pyright over tests/; resolve() is typed Series | DataFrame, so the bare subscript assert read as a Series[bool]. Assert isinstance Series first. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…solver InChIKey blocks Promote three already-reviewed resolver fixes from fork/dev to Phenome-Health dev, so the production engine the preprint cites contains them. Codebase-only slice of the benchmark work; the studies/ harness that exercises these lands separately. - utils.py: retry bulk_kestrel_request on transient 5xx / connection errors (was trentleslie#29) - models.py: coerce Series NaN to None in Entity, unblocking the Phase-0 gate smoke (was trentleslie#37) - core/structure_resolver.py: additive inchikey_blocks() helper for structure-oracle scoring (was trentleslie#36) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`main` and `dev` had diverged: #74 (test markers, tiered scripts, release-please, ci.yml, kg-regression.yml) landed on `main`, while #75 and #77 landed on `dev`. Neither was an ancestor of the other, so a `dev` -> `main` promotion would have dropped #74. This merges `main` into `dev` so `dev` is a superset and the normal direction works again. One conflict, in `bulk_kestrel_request`: * `dev` (#77) added a 5xx retry ladder with exponential backoff, reading the import-time `KESTREL_API_URL` constant. * `main` (#74) has no retry, but resolves the URL through `get_kestrel_api_url()` so the `--kestrel-url` option can override it after import. Both are kept: the retry ladder stays, and its one URL line now calls `get_kestrel_api_url()`. Taking either side verbatim would have been wrong -- `dev`'s side references `KESTREL_API_URL`, which the merged import block (from `main`) no longer imports, so it would have been a NameError at request time. Verified on the merge result: * `ruff check` clean. * `pytest -m "not requires_api and not third_party and not performance"`: 250 passed, 62 deselected. `main` alone is 233 passed, so the merge adds 17 passing tests and no failures. * `pyright`: no new diagnostic attributable to the resolution. The two `utils.py` diagnostics present afterwards are present identically on both parents.
…ken 2.1.0 normalizer) The original reconcile merged main at 96931cf (#74). #80 landed on main on 08-18, after this branch was cut, so dev would still have come out without the Kraken 2.1.0 normalizer. Re-merging main at 0bd9b50 folds #80 in, so #79 now brings both #74's test tiers/CI and #80's 2.1.0 client adaptation to dev in one promotion. No conflicts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ci.yml arrived on dev via this reconcile, but its triggers are scoped to `branches: [main]` for both push and pull_request — so PRs against dev would still have had zero checks after merging this. Add dev to both lists. live-kestrel stays main-only. It is continue-on-error/informational, and firing it on every dev PR multiplies calls against an external service for signal that blocks nothing. dev PRs get the offline gate, which is the one that matters. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…act the cache Promotes the security-relevant slice of the fork's `dev` into the org repo. All of this is already merged and reviewed on trentleslie/biomapper2 `dev` (PRs #46-#53); this PR is path-scoped to the client and its config so the fixes land ahead of the much larger studies/ promotion. Three defects, each with a test that fails without the fix: 1. **The API key could be sent to a third-party host.** `KESTREL_API_URL` now defaults to the public, keyless Kestrel, and `kestrel_host_accepts_credentials` refuses to attach the credential to that host at all — compared by normalized hostname, failing closed on an unparseable URL. `_KestrelCachedSession` also scrubs the header on a cross-origin redirect, which `requests` does only for `Authorization`. 2. **The key was persisted to disk in cleartext.** `requests_cache`'s default ignored-parameter list matches case-sensitively and we send `X-API-Key`; that one-character mismatch wrote the credential into every cached record. `CACHE_IGNORED_PARAMETERS` enumerates the casings actually sent, and `CACHE_DIR` is now 0700. 3. **No default request timeout.** The kwarg was forwarded but never defaulted, so mapping-path callers had none and a wedged request could hang a run forever. The value is derived from the observed successful-request duration distribution rather than chosen — `studies/analysis/request_timeout.py` and its committed artifact are included precisely so the test can assert the constant against the derivation instead of against taste. Also here because they live in the same function and cannot be separated without inventing a state that was never green: a retry ladder for transient 5xx/connection errors, per-endpoint request counters, and a DORMANT bisect-on-5xx path (`KESTREL_BISECT_ON_5XX_ENABLED = False`, budgets counted in request volume, every cap failing loud). `get_kestrel_api_key` is no longer memoized — the old cache keyed on `is None`, so it froze the value for the process lifetime and made results depend on call order. Two deltas from the fork's copy, both deliberate: - `pyproject.toml` gets `pythonpath = ["."]` so `studies.analysis` resolves. The fork's line is identical; only the comment differs. - `utils.py:631` drops a quoted annotation that trips UP037. The fork is lint-red on this line and should take the same fix. Verification: `uv run ruff check` clean, `uv run pytest -m "not external"` 329 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t goes to Rebasing this onto #79 put the host guard and the request URL on different sources. #74 made the backend resolvable AFTER import (kg-regression's --kestrel-url sets the env late), so the client builds its URL from get_kestrel_api_url() -- but the guard added here still judged the import-time KESTREL_API_URL constant. Split those two and the guard makes a decision about one host while the request goes to another: override to the public endpoint after import and the constant still names the internal host, so the key is sent to the public host. That is the exact leak this branch exists to close, and the merge would have re-opened it silently. Resolve the URL once per call and use it for all three: the credential decision, the request, and the 401/403 remediation hint. The withheld-key warning now takes the URL it was withheld from rather than reading the constant, so it names the right host under an override. test_kestrel_auth_header now patches the RESOLVER rather than the constant -- pinning a value production no longer consults is not a test. Adds a regression guard asserting the guard-judged URL and the sent-to URL agree; it is structural rather than header-based on purpose, because a header assertion passes whenever the constant and resolver happen to agree, which is the case on the default config. Also clears this branch's pre-existing lint debt, which was invisible until #79 gave dev a CI gate: a duplicate 'import pytest' from the merge, an incompatible log_message override, and 17 pyright argument-type errors on the duck-typed transport doubles (scoped file-level suppression, rationale in the file). Verification: ruff clean, black clean, pyright 0 errors, 302 passed / 62 deselected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…he constant With KESTREL_API_URL unset, config.KESTREL_API_URL returned the PUBLIC host while get_kestrel_api_url() returned the INTERNAL one. The client resolves through the function, so dev has been defaulting to kestrel.nathanpricelab.com since #78 merged -- the public-default promotion that #78 exists to deliver was cosmetic. Two independent readings of one setting. get_kestrel_api_url() was added by #74 with its own hardcoded fallback, correct at the time because the repo default WAS internal. #78 promoted the default to public by moving the constant, and nothing connected the two, so they silently disagreed. Drop the private _DEFAULT_KESTREL_API_URL and fall back to PUBLIC_KESTREL_API_URL, leaving exactly one place that decides the default. Env override is unaffected: KESTREL_API_URL=... still wins, and --kestrel-url still reaches the client after import. Adds a test asserting the constant and the resolver agree under a cleared environment. Verified it fails against the pre-fix code with 'https://kestrel.nathanpricelab.com/api' != 'https://kestrel.krakenkg.com/api'. ruff clean, black clean, pyright 0 errors, 303 passed / 62 deselected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…okup-status plumbing The correctness-layer changes the resolution certificate is built on. No mapper wiring here — that lands in the certificate PR on top of this. linker: * get_equivalent_ids_checked() returns (mapping, ok). An empty /get-nodes payload otherwise conflates "the graph lists no equivalent ids" with "the call raised and we swallowed it". The certificate must not read an outage as structure_absent — no offline rerun on the resulting TSV could tell the two apart. get_equivalent_ids() keeps its signature, delegating to the checked form. resolver: * is_small_molecule() — public delegate over the small-molecule subtree test, so a caller outside resolution (the certificate) can tell a metabolite row, where "the graph asserts no structure" is a real refusal, from a gene row, where it is a category error. One definition, two callers. * RefMet multi-node handling: when RefMet contributes >1 KG node, sort deterministically and warn rather than picking arbitrarily, and test the majority against the whole RefMet set (not just node[0]) so a spurious divergent_refmet is not emitted when the majority IS one of the RefMet nodes. structure_resolver: * connectivity_match compares InChIKey connectivity blocks via inchikey_blocks, keeping the MW/PubChem name fallback so conflict_no_structure cannot fire when a structure was in fact retrievable; percent-encode names on the outbound REST paths (safe="") so a slash in a compound name cannot inject a path segment. ruff clean, black clean, pyright 0 errors, offline gate 316 passed / 62 deselected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…, on-category guard
PR of 3 bringing the fork's functional work to dev. The annotator layer —
independent of the resolver/linker PR and of the certificate PR; no mapper wiring.
New annotators:
* goslin_grammar / goslin_lipid — a SmallMolecule route where parse-success IS the
lipid detector. pygoslin canonicalizes messy/dialect shorthand (formula, mass,
dialect) offline; a parse miss returns {} so a non-lipid falls through to the
other annotators unchanged. The canonical name is then handed to the existing
RefMet name binder, which previously failed on the raw shorthand and now matches.
Adds pygoslin>=2.0.
* lipidmaps_rest — an INJECTED, off-by-default enrichment seam (LM_ID/InChIKey),
kept out of the default path to avoid the circular LMSD route; the metadata
records when it fired.
annotation_engine / base:
* Registers the Goslin route and threads an accepted_categories set derived from
CATEGORY_ACCEPTED_ROOTS. is_on_category() is a correctness guard on the committed
node's Biolink category, NOT a re-ranking preference — independent of
prefer_canonical/prefer_human, so it has no request-level flag on purpose.
* kestrel_{hybrid,text,vector} and metabolomics_workbench carry the supporting
annotator changes.
Test-double typing: the fork-only tests use duck-typed fakes (a name binder, a
Biolink client, a session) that implement only the surface under test. Constructing
the real collaborators would hit the network. Scoped file-level suppression where a
fake recurs (goslin_lipid), inline ignores for the singletons, and a real
None-guard for the float|None mass assertion.
ruff clean, black clean, pyright 0 errors, offline gate 332 passed / 62 deselected.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ctness feat(resolver,linker): source-weighted resolution + lookup-status plumbing
…idmaps feat(annotators): Goslin lipid-shorthand route + on-category guard
…apper paths PR 3 of 3 — the certificate feature, sitting on top of the resolver/linker PR (uses is_small_molecule + get_equivalent_ids_checked). STACKED on feat/resolver-linker-correctness; retarget to dev once that merges. certificate.py (new): a frozen ResolutionCertificate — what the graph asserts about chosen_kg_id (and ONLY chosen_kg_id), with a structural state machine (uncorroborated / corroborated / conflict / structure_absent / ...). Replaces the ast.literal_eval-only review column with flat scalar columns that survive a TSV round-trip. derive_chosen_kg_id_review() derives the legacy flag FROM the certificate so the two can't disagree. tier_b.py (new): opt-in, default-OFF independent structure lookup (Metabolomics Workbench + PubChem by name), used only to corroborate — never to resolve. Gated so a test suite can't fire it by accident; the single committed sweep is a separate supervised step. mapper.py: issues the certificate on BOTH the single-entity and batch paths through one shared _issue_certificate(), so the two surfaces can't drift. The lookup is scoped to rows the certificate can be about (small-molecule, in population) rather than called-and-discarded. models/api carry the certificate as a plain dict — never the dataclass — because both emission surfaces json.dumps it, and on the streaming path that dumps sits outside the try/except. Also fixes a latent bug the certificate exposed: the batch path assigned a list[str | None] review column, which pandas coerces to a float NaN column where later `is None` identity checks silently miss. Now an explicit object-dtype Series. Test-double typing: certificate tests inject stub collaborators into a real Mapper / StructureResolver and spread kwarg-dicts into issue(); scoped file-level suppressions (rationale in each file), plus real fixes where cleaner (a str() coercion, an object-dtype Series). ruff clean, black clean, pyright 0 errors, offline gate 634 passed / 216 skipped / 62 deselected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e-impl feat(certificate): resolution certificate + Tier B, wired into both mapper paths
This was referenced Sep 17, 2026
This branch was previously deployed
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.
Promotion of⚠️ below).
devtomain— 23 commits. Staged for review; DO NOT MERGE until the production deploy is confirmed safe (seePush to
mainfires three automatic workflows:deploy-api.yml—git reset --hard origin/main,uv sync, restartbiomapper2-apion the Lightsail box (environment: production). Every path filter (src/biomapper2/**,pyproject.toml,uv.lock) matches this diff, so it will deploy.mirror-sync.yml— force-pushesmainto the public mirrorarpanauts/biomapper2. This publishes all the code below publicly.feat:/fix:commits;0.2.0(or higher) supersedes the current chore(main): release 1.0.0 #81.The one behavior change to confirm first: Kestrel default flips to PUBLIC
maintoday:get_kestrel_api_url()→https://kestrel.nathanpricelab.com/api(internal)https://kestrel.krakenkg.com/api(public)If the prod
biomapper2-api.envdoes not setKESTREL_API_URL, the deployed API switches KG endpoint on restart. Confirm the prod.envpinsKESTREL_API_URL, or that switching to public is intended, before merging. Everything else in this promotion is additive.What it carries
Security hardening (#78, #82) — withhold the Kestrel key from the public host, redact the on-disk cache, and default to public Kestrel with a single source of truth for the default (the #82 fix that stopped the constant and the resolver from disagreeing).
Resolver / linker correctness (#75, #77, #83) — source-weighted small-molecule resolution toward RefMet, layered InChIKey StructureResolver,
get_equivalent_ids_checked(outage vs empty-graph),is_small_molecule, RefMet multi-node determinism, 5xx retry + NaN coercion.Annotators (#84) — Goslin lipid-shorthand route (
pygoslin), off-by-default LIPID MAPS enrichment,is_on_categorycorrectness guard.Resolution certificate (#85) — the
ResolutionCertificatestate machine + flat TSV columns, opt-in default-off Tier B corroboration, issued on both mapper paths through one shared issuer.Infra (#79) — the CI
gate(ruff/black/pyright/offline tests) now ondevand firing on dev PRs.Verification
Each constituent PR merged green. Current
devHEAD: ruff clean · black clean · pyright 0 · offline gate 663 passed / 216 skipped / 62 deselected.Not included (stays on the fork)
studies/(benchmarking, ~57k lines) and its 8 coupled test files — functional code only, per the migration plan.🤖 Generated with Claude Code