feat(worker): flag-gated medspaCy assertion tagging + offline eval gate#615
Conversation
Add WORKER_MEDSPACY_ASSERTION (default off): when enabled, chunk texts are batched through worker/python/analyze_assertions.py (medspaCy ConText with clinical-vocabulary target terms) and per-chunk negation/uncertainty/family/ historical annotations land under document_chunks.metadata.assertion — the existing jsonb column, so no schema change or drift-manifest churn. Fail-open contract throughout: a broken or missing medspaCy install degrades to unannotated chunks and can never fail an ingestion job. The medspacy prerequisite check runs only when the flag is on, so existing workers keep starting cleanly. Nothing consumes the annotations yet — answer-verification is deliberately NOT wired to them. Eval-first acceptance gate: scripts/eval-assertions.ts + a 30-case AU community-mental-health golden fixture (npm run eval:assertions, local Python only, no providers). Baseline run scored 95.2% (negation 12/12, uncertainty 7/7, family 4/4, historical 3/3) after adding AU ConText cues the default US-leaning rules miss (nil, query, suspected, may represent, differential diagnosis includes, previous episode of; forward cues scope-bounded). The two remaining known failures are conjunction-scope over-negation — conservative for a safety feature (suppresses evidence rather than asserting a negated finding). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds an optional, fail-open medspaCy assertion-tagging path for worker ingestion, including Python analysis, TypeScript orchestration, prerequisite checks, metadata integration, configuration, tests, and an offline golden-fixture evaluator. ChangesAssertion tagging
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Worker
participant AssertionTagger
participant PythonAnalyzer
participant MedspaCy
participant ChunkMetadata
Worker->>AssertionTagger: Submit chunks and vocabulary targets
AssertionTagger->>PythonAnalyzer: Write input and spawn analyzer
PythonAnalyzer->>MedspaCy: Build ConText pipeline and classify targets
MedspaCy-->>PythonAnalyzer: Return assertion categories
PythonAnalyzer-->>AssertionTagger: Return JSON payload
AssertionTagger-->>Worker: Return assertions by chunk ID
Worker->>ChunkMetadata: Merge assertion metadata
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e16691fef7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex resolve actionable Codex review findings for this pull request and current head using the repository instructions. This is the pull request's single automatic repair pass: do not perform a fresh review, create new standalone findings, or request another review. Work only the existing unresolved Codex threads on the current head. Always fix P0 and P1 findings. For P2 and lower findings, fix only clear, scoped, low-risk issues; otherwise disposition them with a concise reason. After fixing or dispositioning a thread, reply in that thread with as the first line, followed by a concise summary; that marker authorizes the workflow to close that exact thread. If human input or new authorization is required, do not use the marker and leave the thread open with the blocker. Finish only after every actionable thread is fixed or dispositioned and closed, or explicitly left open for a human decision. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
worker/python/analyze_assertions.py (1)
47-52: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftModel reload per document — no warm pipeline reuse.
build_nlp(spaCy/medspaCy load + AU context rules) runs fresh for everyanalyze_assertions.pyinvocation, and the TS caller invokes it once per document. OnceWORKER_MEDSPACY_ASSERTIONis enabled at scale, this adds a full model-load cost (typically seconds) per document on top of ingestion. Worth considering a long-lived process (persistent daemon/IPC) or caching if this feature graduates beyond eval-only use.🤖 Prompt for 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. In `@worker/python/analyze_assertions.py` around lines 47 - 52, Update the analyze_assertions workflow around build_nlp to reuse a loaded medspaCy pipeline across documents or invocations instead of rebuilding it per document. Cache the initialized pipeline, including target matcher and AU context rules, while preserving target-specific matcher configuration and existing assertion-analysis behavior.worker/main.ts (1)
1432-1443: 🧹 Nitpick | 🔵 TrivialVerify production-readiness check before merge.
This wires a new subprocess-based enrichment path into clinical ingestion. As per coding guidelines, changes to clinical ingestion should run the smallest relevant domain check plus
npm run check:production-readinessbefore merge — please confirm this was run (with the flag both on and off).🤖 Prompt for 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. In `@worker/main.ts` around lines 1432 - 1443, Before merging the clinical ingestion change around WORKER_MEDSPACY_ASSERTION and annotateChunkAssertions, run the smallest relevant domain check and npm run check:production-readiness with the flag enabled and disabled, then address any failures.Source: Coding guidelines
🤖 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 `@scripts/eval-assertions.ts`:
- Around line 79-112: The clinical acceptance gate only checks required
memberships and must also reject forbidden category labels. In
scripts/eval-assertions.ts, update the membership checks to generate failures
for forbidden buckets while preserving intentional family/historical overlap;
extend scripts/fixtures/assertion-golden.json at lines 2-2 to define expected
and forbidden category buckets, and at lines 173-176 require tremor to remain
asserted or explicitly forbid negation and uncertainty.
- Around line 51-53: Update the minAccuracy parsing and validation in the
script’s argument-handling flow to reject non-finite values, including NaN, and
any threshold outside the inclusive [0, 1] range before evaluating or reporting
assertion results. Fail conservatively with a clear validation error instead of
allowing evaluation to print PASS; apply the same validation wherever
minAccuracy is used.
In `@worker/assertion-tagging.ts`:
- Around line 77-115: Update runAssertionScript’s Python subprocess promise to
enforce a short timeout, terminating the spawned child with child.kill() and
rejecting with an appropriate error when exceeded. Clear the timer when the
process exits or fails to start, while preserving the existing stdout, stderr,
exit-code handling, cleanup, and fail-open behavior.
---
Nitpick comments:
In `@worker/main.ts`:
- Around line 1432-1443: Before merging the clinical ingestion change around
WORKER_MEDSPACY_ASSERTION and annotateChunkAssertions, run the smallest relevant
domain check and npm run check:production-readiness with the flag enabled and
disabled, then address any failures.
In `@worker/python/analyze_assertions.py`:
- Around line 47-52: Update the analyze_assertions workflow around build_nlp to
reuse a loaded medspaCy pipeline across documents or invocations instead of
rebuilding it per document. Cache the initialized pipeline, including target
matcher and AU context rules, while preserving target-specific matcher
configuration and existing assertion-analysis behavior.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 879b7ae9-6f4e-4870-b65b-280c6ed6a23a
📒 Files selected for processing (11)
.env.examplepackage.jsonscripts/eval-assertions.tsscripts/fixtures/assertion-golden.jsonsrc/lib/env.tstests/assertion-tagging.test.tsworker/assertion-tagging.tsworker/main.tsworker/prerequisites.tsworker/python/analyze_assertions.pyworker/python/requirements.txt
…threshold validation, subprocess timeout Review findings from PR #615 (Codex + CodeRabbit), all verified valid: - The eval only scored positive expectations, so an over-broad ConText rule flagging extra targets negated/uncertain passed silently. Every target not expected in an existence-changing category must now be absent from it (family/historical stay positive-only: contextual qualifiers may co-occur). The stricter gate immediately caught two real cases: "rather than agitation" (correct exclusion semantics — encoded in the fixture with a note) and "Possible NMS; cease antipsychotic" scope leaking across a semicolon (fixed with a TERMINATE rule at clause boundaries). - --min-accuracy now rejects non-finite or out-of-range values instead of silently disabling the gate (accuracy < NaN is always false → false PASS). - runAssertionScript enforces a 120s timeout with child.kill() so a hung Python process rejects into the fail-open path instead of wedging ingestion. Eval after hardening: 98.0% on 101 checks (was 95.2% on 42) — negated 13/13, uncertain 8/8, zero false uncertainty; the two remaining failures are the documented conjunction over-negation limitation (conservative direction). Deliberately NOT terminating scope at "and": that would under-negate "no fever and rigors", the clinically unsafe direction. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ebe88bdc8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| async function main() { | ||
| const fixturePath = argValue("--fixture") ?? path.join("scripts", "fixtures", "assertion-golden.json"); | ||
| const minAccuracy = Number(argValue("--min-accuracy") ?? "0.8"); |
There was a problem hiding this comment.
With the default --min-accuracy of 0.8, npm run eval:assertions exits 0 even when up to 20 of the 101 current fixture checks fail; the later fail condition only looks at aggregate accuracy. Fresh evidence after the earlier assertion-flag comment is that the new false-positive checks are now counted, but a ConText change that misses one or several golden negation/uncertainty cases can still print PASS and bless incorrect clinical assertion metadata before WORKER_MEDSPACY_ASSERTION is enabled. Make the default gate require zero failures, or require an explicit non-default threshold for experimental runs.
Useful? React with 👍 / 👎.
1 participant
Summary
Adds clinical assertion tagging (negation / uncertainty / family / historical context) to the ingestion worker behind a new
WORKER_MEDSPACY_ASSERTIONflag — default off, fully inert on live ingestion. This is the eval-first foundation for eventually distinguishing "no suicidal ideation" from an asserted finding in the answer pipeline; nothing consumes the annotations yet andanswer-verification.tsis deliberately untouched.worker/python/analyze_assertions.py(medspaCy 1.3.1, MIT): ConText assertion detection over caller-supplied target terms, mirroring theextract_pdf_assets.pysubprocess contract (file-first output, stdout fallback, distinct exit codes).worker/assertion-tagging.ts: Zod-validated wrapper with an injectable runner; fail-open contract — any script/parse/validation failure logs a warning and the ingestion job proceeds unannotated.document_chunks.metadatajsonb under a namespacedassertionkey — no schema change, no migration, no drift-manifest churn.nil,query X,suspected,may represent,differential diagnosis includes,previous episode of. Added with bounded forward scopes.Eval evidence (
npm run eval:assertions— local Python only, no providers)30-case AU community-mental-health golden fixture (
scripts/fixtures/assertion-golden.json), 42 term-level checks:The two remaining failures are a single known class — negation scope crossing a conjunction ("no neutropenia and the ANC remains within range" over-negates the second clause). Both err conservatively (over-suppression, never asserting a negated finding), which is the acceptable failure direction for a clinical-safety feature.
Verification
tests/assertion-tagging.test.ts(8 tests, mocked subprocess — CI has no Python): payload validation, id-keyed mapping, fail-open on throw/garbage, subprocess skip on empty input, vocabulary target sourcing.Acceptance gate before any consumption (future PR, not this one)
npm run eval:assertionsoutput against real corpus samples.metadata.assertionon real chunks.metadata.assertioninto answer verification/ranking.Clinical governance preflight
Touches ingestion, gated off by default: flag unset ⇒ byte-identical worker behavior. With the flag on, the only write-path change is an additive namespaced jsonb key;
search_tsvdoes not indexmetadata, so retrieval/ranking are unaffected. Fail-open ensures no new ingestion failure mode.🤖 Generated with Claude Code
Summary by CodeRabbit
WORKER_MEDSPACY_ASSERTIONfeature flag (off by default).eval:assertionsscript to run offline tagging accuracy checks.