Release v0.3

[pendingintent]

cdisc-concept-curation Improvement Plan

Context

A full analysis of /Users/dmoreland/projects/cdisc-concept-curation (Flask 3 +
Flask-SQLAlchemy Biomedical Concept curation app: ingest → SME review → governance
→ publish) found a functionally complete app (branch release-v0.2, ~164 tests,
clean test isolation) with several classes of debt:

• Codebase: zero logging anywhere; all three external-API clients
  (services/cdisc_api.py, ncit_api.py, loinc_api.py) swallow exceptions into
  {"error": ...} return values; dual schema management (db.create_all() in
  app.py coexisting with a 4-revision Alembic chain that has no baseline and an
  inconsistent code/loinc_code rename/re-add); 6 duplicated audit-log blocks in
  routes/bc.py (373 LOC); untested services/export.py,
  routes/specializations.py, routes/dashboard.py, and the cdisc_api cache;
  no type hints; stale committed results.txt; doc drift (README/CLAUDE.md say 7
  blueprints and port 5000; reality is 8 blueprints and port 8081).
• Claude config: mike.md agent is a mis-templated copy from another project
  ("Kanojo") referencing non-existent files/agents; CLAUDE.md omits the LOINC
  feature and never states the test command; zero project skills; orphaned
  agent-memory/cdisc-concept-explorer/ with verified API docs but no agent;
  cdisc-frontend-dev memory contradicts CLAUDE.md on two facts; settings.json
  has broad Write(*)/Edit(*) grants and a Stop hook that auto-rewrites
  README.md (conflicts with the recorded "ask before edits" preference).
• MCP: no server exists. The soa-workbench server
  (src/soa_builder/mcp/server.py: raw MCP SDK, _TOOLS + _dispatch seam,
  stdio, direct-dispatch tests) is a proven pattern to mirror; this project's
  services layer is callable outside the request context.

User decisions: MCP read-only tools first then writes; codebase quick wins + the
Alembic schema fix (auth/actor identity deferred); rewrite the mike agent for this
project; keep the Stop hook but narrow it.

Conventions: TDD (tests before implementation, per project memory), pre-commit
runs black/flake8/pytest, CI runs pytest --tb=short on Python 3.12.

---

Workstream A — Claude config & docs (small, do first)

1. Fix CLAUDE.md drift
   • Add the loinc blueprint (8 total, /loinc, services/loinc_api.py,
     tests/test_loinc.py); correct the "7 blueprints" count.
   • Correct default port 5000 → 8081 (config.py).
   • Add a Testing section: pytest --tb=short, in-memory SQLite via
     tests/conftest.py TestConfig, pre-commit hooks (black/flake8/pytest), CI
     workflow (.github/workflows/ci.yml, Python 3.12).
2. Rewrite mike.md for this project: PM assistant anchored on
   README-PROGRESS.md (which exists); remove Kanojo references, the
   non-existent docs/modules/* paths, and the phantom teammate agents; reference
   the real agent roster (cdisc-frontend-dev, plus the new concept-curation
   agent below).
3. Adopt the orphaned agent memory: create a project-level
   cdisc-concept-explorer agent (or a thin project variant referencing the
   user-level one) so agent-memory/cdisc-concept-explorer/reference_api_endpoints.md
   (verified COSMOS v2 endpoint docs) is actually loaded; generalize the
   user-level cdisc-concept-explorer.md and ncit-concept-resolver.md wording
   so they apply to both soa-workbench and this project.
4. Reconcile cdisc-frontend-dev memory: fix project_foundation.md
   (ingestion queue is the IngestionRecord DB table, not session; cdisc_api.py
   is a full client, not a stub; add the loinc blueprint).
5. Narrow the Stop hook in .claude/settings.json: instead of rewriting
   README.md wholesale, have it update only README-PROGRESS.md's changelog
   section; keep the 120s timeout.
6. Tidy settings.json: remove inert VS Code keys (files.associations,
   emmet.includeLanguages); with Write(*)/Edit(*) present, drop the
   redundant per-file grants (or narrow the broad grants if least-privilege is
   preferred — default: drop redundant granular entries, keep broad).
7. New project skills (mirror soa-workbench's proven set):
   • .claude/skills/run-concept-curation/SKILL.md + smoke.sh: venv activate,
     python app.py (port 8081, CDISC_API_KEY env), curl checks against /,
     /bc/, /governance/board, /ncit/search, teardown; use a throwaway
     DATABASE_URL sqlite path, never instance/cdisc_curation.db.
   • .claude/skills/code-review/SKILL.md: project-specific checks — route→
     service→model layering, audit-log coverage on mutations, API-client error
     contract, TDD, Alembic migration for any model change.

Workstream B — Codebase quick wins + schema fix

1. Logging + error-handling overhaul (services/*.py, app.py)
   • Add logging config in app.py/config.py; module loggers in each service
     and route file.
   • Keep the {"error": ...} return contract for now (templates depend on it)
     but log every caught exception with context (URL, status) at error level;
     narrow except Exception to requests.RequestException + JSON/parse errors
     where evidence allows (12 sites, all in services).
2. routes/bc.py de-duplication
   • Extract the 6 repeated AuditLog(...) + db.session.add + commit blocks into
     a services/audit.py log_change(entity_type, entity_id, action, before, after, actor) helper (this also becomes the shared write path for MCP
     milestone 2).
   • Consolidate the duplicated request.form.get(...) field mapping in
     create()/edit() into one form-to-model mapper.
3. Schema management fix (approach being finalized — see Workstream D notes):
   single source of truth = Alembic; proper baseline; existing DBs stamped; fresh
   DBs created via flask db upgrade; tests keep create_all() in conftest.
4. Close test gaps (TDD-style, before touching the code in 1–2):
   tests/test_export_service.py (json/xlsx/odm-xml round-trips),
   tests/test_specializations_routes.py, tests/test_dashboard.py
   (ThreadPoolExecutor path with mocked client), tests/test_cdisc_api_cache.py
   (fresh/stale/error states of _cached()).
5. API-client consistency (services/)
   • NCItApiClient: honor NCIT_API_BASE_URL from config instead of the
     hardcoded class constant.
   • Adopt soa-workbench's dual-header auth in CDISCApiClient
     (CDISC_SUBSCRIPTION_KEY/Ocp-Apim-Subscription-Key preferred,
     CDISC_API_KEY/api-key fallback) for parity across projects.
   • Add simple client-level caching to loinc_api.py matching the shared
     _cached() helper; unify the bespoke _ncit_cache onto the same helper.
6. Hygiene
   • git rm results.txt (stale committed pytest log); remove empty src/ dir.
   • Gate debug=True in app.py behind an env var (FLASK_DEBUG).
   • Split requirements-dev.txt (pytest/black/flake8/pre-commit) from runtime
     requirements.txt; add black/flake8 steps to CI; wire the configured isort
     into pre-commit.
   • Tighten .flake8: drop F401,F841 from the ignore list (fix fallout),
     reduce max-line from 999 to something enforceable (match black's configured
     line length).

Workstream C — MCP server (new)

Mirror the soa-workbench pattern (raw MCP SDK, _TOOLS list, _dispatch dict,
stdio transport, tests call _dispatch directly). Handlers run inside
create_app().app_context() and use the ORM so audit behavior stays consistent.
Exact package layout/entry point per Workstream D design notes.

Milestone 1 — read-only tools (7):
list_bcs (q/status/pagination), get_bc (BC + DECs + specializations +
governance history via to_dict()), search_ncit, get_ncit_concept,
search_loinc, search_cdisc_library + get_library_bc, list_review_queue
(governance board + pending IngestionRecords). Reuse
services/{ncit_api,loinc_api,cdisc_api}.py directly.

Milestone 2 — write tools (6), after milestone 1 is validated against the
shared SQLite file: create_bc, update_bc, map_ncit_to_bc,
submit_bc_for_review, advance_governance, reject_bc. Prerequisite: extract
the write+audit logic from routes/bc.py/routes/governance.py/routes/ncit.py
into shared service functions (building on the log_change helper from B.2) so
routes and MCP share one code path.

Register in a new .mcp.json; add mcp>=1.0.0 to requirements;
tests/test_mcp_server.py with direct _dispatch calls, test-config app context,
mocked external APIs (matching existing conftest patterns).

Workstream D — Design notes (verified against both repos)

D.1 Alembic baseline — squash to a clean initial migration

Verified facts: the live DB (instance/cdisc_curation.db) is stamped at head
c2d4e6f8a0b1 and its schema exactly matches current models. flask db upgrade
is broken on fresh DBs in two independent ways: (1) the Flask-Migrate app factory
runs db.create_all() inside create_app(), so the first revision fails with
"duplicate column"; (2) no revision creates the base tables, and the
code→loinc_code rename revision assumes a historical schema unreproducible
from the current codebase. Tests are unaffected by removing create_all —
tests/conftest.py's clean_db fixture owns table creation.

Approach (do this BEFORE the MCP server — it makes create_app() side-effect
free):

1. Delete the db.create_all() block from create_app() in app.py; the
   __main__ block calls a new bootstrap helper before app.run(...).
2. New db_bootstrap.py (repo root) with ensure_db(app):
   • legacy create_all DB (tables exist, no alembic_version) → stamp() to
     new head;
   • DB stamped at any of the 4 legacy revision ids → restamp to new head
     (schema already matches);
   • otherwise → flask_migrate.upgrade() (no-op when current; builds fresh
     DBs). Unrecognized intermediate states raise with a clear message
     (manual fallback: flask db stamp head, documented in README).
3. Delete all four files in migrations/versions/.
4. Regenerate one baseline revision (after step 1):
   DATABASE_URL=sqlite:////tmp/baseline_gen.db FLASK_APP=app.py flask db migrate -m "baseline: initial schema" — review that it creates all six
   tables and nothing else. The code/loinc_code churn ceases to exist.
5. Risk control: diff .schema of a create_all() DB vs a flask db upgrade
   DB before committing (constraint-naming differences are possible).

D.2 MCP server — python -m mcp_server, app-context handlers

Packaging: do NOT add [project] metadata (flat top-level modules make
setuptools discovery churn for zero benefit). New top-level package
mcp_server/ (not mcp — would shadow the SDK): __init__.py,
server.py (mirrors soa-workbench: _TOOLS, _dispatch, sync handlers via
run_in_executor, stdio main()), __main__.py. Add mcp>=1.0.0 to
requirements. New .mcp.json at repo root launching
.venv/bin/python -m mcp_server with CDISC_API_KEY in env.

App/DB sharing: lazy _get_app() singleton importing create_app() (safe once
D.1 lands); each handler pushes with _get_app().app_context(): inside the
handler (handlers run on executor threads) — implement as a decorator so it
can't be missed. instance_path resolves identically in both processes, so
Flask and MCP share the same SQLite file with no extra config; DATABASE_URL
stays the override. WAL: leave journal mode alone for read-only milestone 1;
at milestone 2 add a SQLAlchemy connect listener in extensions.py issuing
PRAGMA journal_mode=WAL; PRAGMA busy_timeout=15000 (required — two writer
processes otherwise hit "database is locked").

Milestone 1 tools (8 read-only): list_bcs, get_bc, search_ncit,
get_ncit_concept, search_loinc, search_cdisc_library, get_library_bc,
list_review_queue. All three API clients verified context-safe.

Milestone 2 service extraction: services/bc_service.py (create_bc,
update_bc, save_decs, map_ncit_to_bc, submit_bc_for_review) and
services/governance_service.py (advance_governance, reject_bc) — routes
become thin form→service adapters; MCP handlers call the same functions with
actor defaulting to "mcp" so audit rows distinguish agent writes.
Intentional behavior fix to call out in the PR: routes/ncit.py:resolve
currently writes NO AuditLog — the extracted map_ncit_to_bc adds one.

Tests: tests/test_mcp_server.py calls _dispatch directly (no transport),
injects the existing in-memory-SQLite test app via mcp_server.server._app,
mocks the API clients with monkeypatch per existing conventions, and asserts
AuditLog/GovernanceRecord rows after every milestone-2 write.

Suggested ordering

A (config/docs, unblocks agents) → B.4 (tests first, TDD) → B.1–B.2 (logging,
audit helper) → B.3 = D.1 (schema fix; prerequisite for MCP) → B.5–B.6 →
C milestone 1 → C milestone 2 (service extraction + WAL).

Verification

• pytest --tb=short green after every step; pre-commit run --all-files before
  each commit.
• Schema fix: verify flask db upgrade builds a correct fresh DB (compare
  sqlite_master against a create_all() DB); verify an existing DB copy is
  stamped and upgrades cleanly. Never test against instance/cdisc_curation.db
  itself — use copies.
• Smoke test via the new run-concept-curation skill after B and C changes.
• MCP: tests/test_mcp_server.py for every tool; python -m mcp_server
  starts and waits on stdio; claude mcp list picks up .mcp.json; then a live
  session exercising list_bcs/get_bc.
• Schema fix specifics: DATABASE_URL=sqlite:////tmp/fresh.db FLASK_APP=app.py flask db upgrade builds all six tables; flask db check reports no drift;
  existing-DB copy restamps to the new head via ensure_db.

[Copilot]

Pull request overview

Copilot reviewed 71 out of 72 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

services/bc_service.py:134

• map_ncit_to_bc() can change the BiomedicalConcept primary key (bc.bc_id) when promoting IMPORT_ ids, but it does not update dependent rows (DataElementConcept.bc_id, DatasetSpecialization.bc_id, GovernanceRecord.bc_id, or child BiomedicalConcept.parent_bc_id). This can orphan related records and/or violate FK constraints once an imported BC has DECs or governance history. If promotion is kept, update all referencing rows in the same transaction (and consider guarding against collisions when the target ncit_code already exists).
  templates/library_bc_detail.html:119
• In the CDISC Library BC detail view, bc.href refers to the CDISC Library resource URL, but the label was changed to “NCI Thesaurus HREF”, which is misleading for users.

[Copilot]

Pull request overview

Copilot reviewed 71 out of 72 changed files in this pull request and generated no new comments.

Suppressed comments (10)

templates/library_bc_detail.html:117

• The label "NCI Thesaurus HREF" is misleading here: this template is rendering bc from the CDISC Library API (see routes/bc.py:88+), and bc.href is the CDISC Library link, not an NCIt link.
  services/bc_service.py:134
• When promoting an IMPORT_ BC id to the resolved NCIt code, this can violate the primary-key uniqueness constraint if a BC with the target ncit_code already exists, leading to an unhandled IntegrityError on commit.
  routes/ingestion.py:147
• Rejecting an IngestionRecord mutates its status but does not write an AuditLog entry for that change, leaving an untracked state transition.
  routes/ingestion.py:178
• In approve_all(), setting an IngestionRecord to approved is also a state transition that currently isn’t audited.
  routes/ingestion.py:180
• In approve_all(), the else-branch also marks the ingestion record approved without auditing the ingestion record status change.
  services/bc_service.py:116
• save_decs() deletes and recreates DataElementConcept rows and commits immediately, but this mutation is not recorded in AuditLog. Also, committing inside the service prevents bundling BC + DEC changes into a single atomic transaction (and contradicts services/audit.log_change()’s “caller owns the commit” contract).
  routes/ingestion.py:138
• Approving an IngestionRecord mutates its status but does not write an AuditLog entry for that change, which leaves an untracked state transition in the ingestion pipeline.

This issue also appears on line 145 of the same file.
services/ncit_api.py:21

• _pick_definition() assumes definitions is iterable, but dict.get('definitions', []) will still return None if the upstream payload includes the key with a null value. That would raise a TypeError during iteration and break concept/search rendering.
  routes/ingestion.py:161
• In approve_all(), records with validation errors/duplicates are marked rejected without any AuditLog entry for the ingestion record status change.

This issue also appears in the following locations of the same file:

• line 177
• line 180
  .mcp.json:7

• The PR description says the new .mcp.json should launch the server with CDISC_API_KEY in env, but the current config doesn’t define any env passthrough. If the MCP host doesn’t automatically inherit the parent environment, external API tools may fail unexpectedly.

    "cdisc-curation": {
      "command": ".venv/bin/python",
      "args": ["-m", "mcp_server"],
      "cwd": "."
    }

[Copilot]

Pull request overview

Copilot reviewed 71 out of 72 changed files in this pull request and generated no new comments.

Suppressed comments (5)

templates/library_bc_detail.html:117

• The metadata label says "NCI Thesaurus HREF", but this page is rendering a CDISC Library concept (routes/bc.py passes bc from CDISCApiClient().get_bc()), so bc.href is a CDISC Library URL/path. This label is misleading for users.
  routes/specializations.py:82
• Creating a specialization doesn't check whether the VLM Group ID already exists. Submitting the form twice (or colliding with an existing ID) will raise an IntegrityError on commit and return a 500 instead of a user-facing message.
  routes/dashboard.py:48
• Dashboard now renders the full CDISC Library BC/spec lists into the HTML and paginates purely client-side. If the Library returns a large list, this can significantly increase response size and slow down rendering, even though only 20 rows are shown at a time.
  routes/ingestion.py:176
• This adds another inline AuditLog(...) construction (approve_all), even though the PR introduces services.audit.log_change() as the shared audit helper. Keeping one helper avoids drift in audit fields and makes future changes (e.g., adding more context) easier.
  README.md:30
• README installation instructions only install requirements.txt, but this PR moves pytest/flake8/black/pre-commit into requirements-dev.txt. As written, pre-commit install will fail (and tests/lint tools won't be available) unless users also install dev requirements.

# 3. Install dependencies
pip install -r requirements.txt

# 4. Install git hooks for code quality
pre-commit install

[Copilot]

Pull request overview

Copilot reviewed 71 out of 72 changed files in this pull request and generated no new comments.

Suppressed comments (3)

routes/specializations.py:150

• generate() also queries DECs without an order_by(), so the generated specialization variable list may come out in an arbitrary order. Ordering by DataElementConcept.sort_order keeps the generated specialization aligned with DEC ordering used elsewhere (including exports).
  routes/specializations.py:83
• _variables_from_form() stops parsing at the first missing index (while variables[i][name] in form). Because the UI lets users delete rows (leaving non-contiguous indices), this will silently drop all later variable rows. Also, the template/JS submits the required checkbox as value="1", but the server only treats "on" as checked, so required will always serialize as False when submitted from the browser.
  routes/specializations.py:141
• generate_from_dec() builds variables from DECs without an order_by(), so the returned variable order is database-dependent. Since DECs already have sort_order, ordering here will make the UI and JSON response deterministic.

This issue also appears on line 149 of the same file.

[Copilot]

Pull request overview

Copilot reviewed 72 out of 73 changed files in this pull request and generated 1 comment.

Suppressed comments (5)

routes/specializations.py:87

• _variables_from_form() stops parsing as soon as variables[0][name] is missing, which will happen if the user deletes the first row (your JS removes rows without renumbering). Also, the checkbox posts value="1" but the backend checks for "on", so required will always be False.
  templates/specializations.html:213
• The list view uses spec.variable_count, but DatasetSpecialization has no such attribute, and this template also compares spec.variable_count != 1 which can behave unexpectedly with an undefined value. Since spec.variables is the source of truth, compute the count from that list.
  routes/dashboard.py:52
• The dashboard now renders all CDISC Library BCs/specializations into the HTML and paginates client-side. If the Library returns hundreds/thousands of rows, this will significantly increase response size and render time (regression vs the prior server-side slice to 50).
  templates/governance.html:66
• The Provisional column no longer renders a Reject button, but the backend still supports POST /governance/reject/<bc_id> (and other columns still show Reject). This makes it impossible to reject a Provisional BC via the UI.
  templates/library_bc_detail.html:69
• The "Long Common Name" fallback uses lc.system (a system URL) when loinc_data.LONG_COMMON_NAME is missing, so the UI can show a URL in a field labeled as a name. Better to fall back to an em dash (or another name-like field) when the LOINC lookup didn't return that attribute.

[Copilot]

Pull request overview

Copilot reviewed 72 out of 73 changed files in this pull request and generated no new comments.

Suppressed comments (4)

templates/specializations.html:213

• spec.variable_count is not defined on DatasetSpecialization, so this cell will always fall back to 0 and the pluralization condition will also evaluate against an undefined value. Use the stored variables list length instead.
  templates/library_bc_detail.html:70
• This "Long Common Name" field falls back to lc.system (a system URL) when loinc_data is empty, which is misleading output for the label. Prefer rendering an em dash (or another explicit placeholder) when the long common name isn't available.
  templates/specializations.html:38
• The edit flow posts back to the same upsert handler keyed by vlm_group_id. Leaving the VLM Group ID field editable makes it easy to accidentally create a new specialization instead of updating the existing one (changing the primary key in the form changes which row is updated). Consider making this input read-only when edit_spec is set.
  README.md:31
• The setup instructions run pre-commit install but only install requirements.txt. In this PR, black/flake8/pytest/pre-commit were moved to requirements-dev.txt, so a fresh dev venv following these steps won't have pre-commit (or its hooks) available. Consider installing both requirements files for development (or explicitly call out the difference between runtime vs dev deps).

# 3. Install dependencies
pip install -r requirements.txt

# 4. Install git hooks for code quality
pre-commit install

</details>

[Copilot]

Pull request overview

Copilot reviewed 72 out of 73 changed files in this pull request and generated no new comments.

Suppressed comments (4)

services/bc_service.py:122

• save_decs() replaces all DataElementConcept rows but does not write an AuditLog entry, so DEC mutations are currently invisible in the audit trail (and callers like routes/bc.py rely on this function for DEC persistence). This breaks the project requirement that every curated-entity mutation is audited. Consider also accepting an actor so MCP writes can record actor="mcp".
  routes/specializations.py:87
• _variables_from_form() assumes the submitted indices are contiguous starting at 0 (while variables[0][name] in form). But the UI allows deleting rows without renumbering, which will create index gaps (e.g. variables[1] exists but variables[0] doesn't) and cause the parser to return an empty/partial list. Also, the template checkboxes use value="1", so checking == "on" will always be false for real form submissions.
  mcp_server/server.py:494
• _create_bc computes an actor (defaulting to "mcp") but does not pass it through to bc_service.save_decs(). Once DEC writes are audit-logged, this will incorrectly attribute DEC changes to the default actor (or lose actor entirely).

    actor = str(args.get("actor") or "mcp")
    bc = bc_service.create_bc(args, actor=actor)
    decs = args.get("decs") or []
    if decs:
        bc_service.save_decs(bc.bc_id, decs)
    return _get_bc.__wrapped__({"bc_id": bc.bc_id})

routes/dashboard.py:52

• The dashboard now renders all CDISC Library BCs/specializations into the HTML (then paginates client-side). If these lists are large, this can significantly increase response size, template render time, and browser work (even though only 20 rows are shown). Consider server-side pagination and/or limiting the number of rows rendered while still showing the total counts.