From 4b61931a444e31e1072eecb0392125d400850cfb Mon Sep 17 00:00:00 2001 From: Agah Date: Sun, 2 Aug 2026 22:13:32 -0400 Subject: [PATCH 01/11] Add myst.yml project metadata translation --- api/myst_frontmatter.py | 125 +++++++++++++++++++++++++++++++++ tests/test_myst_frontmatter.py | 113 +++++++++++++++++++++++++++++ 2 files changed, 238 insertions(+) create mode 100644 api/myst_frontmatter.py create mode 100644 tests/test_myst_frontmatter.py diff --git a/api/myst_frontmatter.py b/api/myst_frontmatter.py new file mode 100644 index 0000000..1e933f6 --- /dev/null +++ b/api/myst_frontmatter.py @@ -0,0 +1,125 @@ +"""Translate a myst.yml `project` mapping into inara paper metadata. + +A NeuroLibre submission declares its title, authors, and affiliations in +myst.yml for the living preprint. The publishing pipeline wants the same +information in the shape the paper.md front matter uses: affiliations numbered +by index, and each author's affiliations as a comma-joined string of those +indices. + +This module is pure. Fetching files is the caller's job, which keeps the +mapping testable without a GitHub client. It mirrors +inara/data/filters/myst-frontmatter.lua; the two share the mapping documented +in the design spec. +""" + +# Parts of a myst.yml affiliation, joined into one name string. Department +# precedes institution to match the convention in existing NeuroLibre front +# matter. +NAME_PARTS = ( + "department", + "institution", + "address", + "city", + "region", + "postal_code", + "country", +) + +# MyST accepts these aliases for two of the parts. +ALIASES = {"institution": "name", "region": "state"} + + +def _affiliation_name(affiliation): + """Join an affiliation's parts into a single display string.""" + parts = [] + for key in NAME_PARTS: + value = affiliation.get(key) + if value in (None, "") and key in ALIASES: + value = affiliation.get(ALIASES[key]) + if value not in (None, ""): + parts.append(str(value).strip()) + return ", ".join(parts) + + +def _affiliation_tokens(value): + """Normalise an author's `affiliations` value to a list of tokens. + + MyST accepts a list, a single id, or several ids in one ';'-separated + string. + """ + if value in (None, ""): + return [] + if isinstance(value, (list, tuple)): + return [str(entry).strip() for entry in value if str(entry).strip()] + return [token.strip() for token in str(value).split(";") if token.strip()] + + +def myst_project_metadata(project): + """Return inara paper metadata derived from a myst.yml `project` mapping. + + Returns only the keys the project actually defines, so the caller can treat + the result as a set of defaults to fill gaps with. Junk input yields an + empty dict: this fallback must never be why a deposit fails. + """ + if not isinstance(project, dict): + return {} + + metadata = {} + + if project.get("title") is not None: + metadata["title"] = project["title"] + if project.get("date") is not None: + metadata["date"] = project["date"] + if project.get("keywords") is not None: + metadata["tags"] = project["keywords"] + if project.get("bibliography") is not None: + metadata["bibliography"] = project["bibliography"] + + affiliations = [] + index_of = {} + for source in project.get("affiliations") or []: + if not isinstance(source, dict): + continue + index = len(affiliations) + 1 + affiliations.append({"index": index, "name": _affiliation_name(source)}) + if source.get("id") is not None: + index_of[str(source["id"])] = index + + authors = [] + for source in project.get("authors") or []: + if not isinstance(source, dict): + continue + author = {"name": source.get("name")} + for target, key in ( + ("email", "email"), + ("orcid", "orcid"), + ("corresponding", "corresponding"), + ("equal-contrib", "equal_contributor"), + ): + if source.get(key) is not None: + author[target] = source[key] + + indices = [] + tokens = _affiliation_tokens( + source.get("affiliations") or source.get("affiliation") + ) + for token in tokens: + index = index_of.get(token) + if index is None: + # MyST permits ad-hoc affiliations. Inventing an entry beats + # dropping the author's affiliation. + index = len(affiliations) + 1 + affiliations.append({"index": index, "name": token}) + index_of[token] = index + indices.append(str(index)) + if indices: + author["affiliation"] = ",".join(indices) + + authors.append(author) + + if authors: + metadata["authors"] = authors + if affiliations: + metadata["affiliations"] = affiliations + + return metadata diff --git a/tests/test_myst_frontmatter.py b/tests/test_myst_frontmatter.py new file mode 100644 index 0000000..f0e4bab --- /dev/null +++ b/tests/test_myst_frontmatter.py @@ -0,0 +1,113 @@ +import pytest + +from api.myst_frontmatter import myst_project_metadata + + +def test_composes_affiliation_name_from_parts_in_order(): + project = { + "affiliations": [ + { + "id": "full", + "department": "Département de génie physique", + "institution": "École Polytechnique de Montréal", + "address": "2500 Chemin de Polytechnique", + "city": "Montreal", + "region": "Quebec", + "postal_code": "H3T 1J4", + "country": "Canada", + } + ] + } + assert myst_project_metadata(project)["affiliations"] == [ + { + "index": 1, + "name": ( + "Département de génie physique, " + "École Polytechnique de Montréal, " + "2500 Chemin de Polytechnique, " + "Montreal, Quebec, H3T 1J4, Canada" + ), + } + ] + + +def test_honours_name_and_state_aliases(): + project = { + "affiliations": [{"id": "a", "name": "Harvard University", "state": "Massachusetts"}] + } + assert myst_project_metadata(project)["affiliations"] == [ + {"index": 1, "name": "Harvard University, Massachusetts"} + ] + + +def test_resolves_affiliation_ids_to_indices(): + project = { + "authors": [ + {"name": "Ada Lovelace", "affiliations": ["engine", "society"]}, + {"name": "Grace Hopper", "affiliations": "society; engine"}, + ], + "affiliations": [ + {"id": "engine", "institution": "Analytical Engine Institute"}, + {"id": "society", "institution": "Royal Society"}, + ], + } + result = myst_project_metadata(project) + assert [a["affiliation"] for a in result["authors"]] == ["1,2", "2,1"] + + +def test_appends_undeclared_affiliation_id_as_literal_name(): + project = { + "authors": [{"name": "Grace Hopper", "affiliations": "Yale University"}], + "affiliations": [{"id": "engine", "institution": "Analytical Engine Institute"}], + } + result = myst_project_metadata(project) + assert result["authors"][0]["affiliation"] == "2" + assert result["affiliations"][1] == {"index": 2, "name": "Yale University"} + + +def test_maps_author_fields(): + project = { + "authors": [ + { + "name": "Ada Lovelace", + "email": "ada@example.org", + "orcid": "0000-0002-1825-0097", + "corresponding": True, + "equal_contributor": True, + } + ] + } + author = myst_project_metadata(project)["authors"][0] + assert author["email"] == "ada@example.org" + assert author["orcid"] == "0000-0002-1825-0097" + assert author["corresponding"] is True + assert author["equal-contrib"] is True + + +def test_maps_scalar_fields(): + project = { + "title": "T", + "date": "03 March 2023", + "keywords": ["photon counting"], + "bibliography": ["content/paper.bib"], + } + result = myst_project_metadata(project) + assert result["title"] == "T" + assert result["date"] == "03 March 2023" + assert result["tags"] == ["photon counting"] + assert result["bibliography"] == ["content/paper.bib"] + + +def test_does_not_map_doi_license_or_venue(): + project = {"doi": "10.55458/neurolibre.xxxxx", "license": {"content": "CC-BY-4.0"}, "venue": "Neurolibre"} + assert myst_project_metadata(project) == {} + + +def test_author_without_affiliations_gets_no_affiliation_key(): + project = {"authors": [{"name": "Ada Lovelace"}]} + assert "affiliation" not in myst_project_metadata(project)["authors"][0] + + +@pytest.mark.parametrize("project", [None, {}, "not a mapping", []]) +def test_tolerates_junk_input(project): + assert myst_project_metadata(project) == {} From ace07c6c87340135239b42b9c0152d93928b4d64 Mon Sep 17 00:00:00 2001 From: Agah Date: Sun, 2 Aug 2026 22:24:00 -0400 Subject: [PATCH 02/11] Fall back to myst.yml for paper metadata on the deposit path --- api/github_client.py | 27 ++++++++- api/myst_frontmatter.py | 48 ++++++++++++++- api/neurolibre_preprint_api.py | 5 +- tests/test_myst_frontmatter.py | 104 +++++++++++++++++++++++++++++++++ 4 files changed, 179 insertions(+), 5 deletions(-) diff --git a/api/github_client.py b/api/github_client.py index 846d360..9f04839 100644 --- a/api/github_client.py +++ b/api/github_client.py @@ -1,9 +1,11 @@ import os import re -from common import get_time +import logging +from common import get_time, parse_front_matter import json import yaml import git +from myst_frontmatter import merge_paper_metadata # Name of the GitHub organization where repositories # will be forked into for production. Editorial bot @@ -330,6 +332,29 @@ def gh_get_paper_markdown(github_client,repo): file_content = gh_get_file_content(github_client,repo,"paper.md") return file_content +def gh_get_paper_metadata(github_client, repo): + """Paper metadata for a submission, with myst.yml filling any gaps. + + NeuroLibre requires myst.yml at the repository root beside paper.md, so a + submission need not repeat its title, authors, and affiliations in the + paper.md front matter. Returns None only when neither source names an + author. + + This runs before the repository is cloned, so both files are fetched + through the GitHub API rather than read from disk. + """ + paper = gh_get_file_content(github_client, repo, "paper.md") + + front_matter = None + if paper: + try: + front_matter = parse_front_matter(paper) + except yaml.YAMLError as error: + logging.warning(f"Could not parse paper.md front matter: {error}") + + myst = gh_get_file_content(github_client, repo, "myst.yml") + return merge_paper_metadata(front_matter, myst) + def gh_read_from_issue_body(github_client,issue_repo,issue_id,tag): """ Issue body of the reviews has markers around review entries diff --git a/api/myst_frontmatter.py b/api/myst_frontmatter.py index 1e933f6..9554d79 100644 --- a/api/myst_frontmatter.py +++ b/api/myst_frontmatter.py @@ -9,9 +9,14 @@ This module is pure. Fetching files is the caller's job, which keeps the mapping testable without a GitHub client. It mirrors inara/data/filters/myst-frontmatter.lua; the two share the mapping documented -in the design spec. +in the design spec. It also merges a parsed paper.md front matter with a +myst.yml, filling any gaps the front matter leaves. """ +import logging + +import yaml + # Parts of a myst.yml affiliation, joined into one name string. Department # precedes institution to match the convention in existing NeuroLibre front # matter. @@ -123,3 +128,44 @@ def myst_project_metadata(project): metadata["affiliations"] = affiliations return metadata + + +def merge_paper_metadata(front_matter, myst_text): + """Paper metadata from paper.md, with myst.yml filling any gaps. + + `front_matter` is the already-parsed paper.md front matter, or None for a + paper that has none. `myst_text` is the raw contents of myst.yml, or None. + Parsing myst.yml happens here rather than in the caller so that a malformed + file is tolerated in one place. + + Returns None when neither source names any author — the same signal the + deposit path already treats as "cannot extract metadata". + """ + metadata = dict(front_matter) if isinstance(front_matter, dict) else {} + + if myst_text: + try: + project = (yaml.safe_load(myst_text) or {}).get("project") + except yaml.YAMLError as error: + logging.warning(f"Could not parse myst.yml: {error}") + project = None + except AttributeError: + # yaml.safe_load returned something that is not a mapping. + project = None + fallback = myst_project_metadata(project) + + # Authors and affiliations are filled as a pair. An affiliation index + # only means something relative to the list that defines it, so mixing + # the two sources would silently attach authors to the wrong + # institutions. + if "authors" not in metadata or "affiliations" not in metadata: + if fallback.get("authors"): + metadata["authors"] = fallback["authors"] + metadata["affiliations"] = fallback.get("affiliations", []) + for key in ("title", "date", "tags", "bibliography"): + if key not in metadata and key in fallback: + metadata[key] = fallback[key] + + if not metadata.get("authors"): + return None + return metadata diff --git a/api/neurolibre_preprint_api.py b/api/neurolibre_preprint_api.py index a86b5f2..663ed6a 100644 --- a/api/neurolibre_preprint_api.py +++ b/api/neurolibre_preprint_api.py @@ -328,11 +328,10 @@ def api_zenodo_post(user,id,repository_url): # We need the list of authors and their ORCID, this will # be fetched from the paper.md in the tarhet repository - paper_string = gh_get_paper_markdown(github_client,repository_url) - paper_data = parse_front_matter(paper_string) + paper_data = gh_get_paper_metadata(github_client,repository_url) if not paper_data: - comment = f"🔴 Cannot extract metadata from the front-matter of the `paper.md` for {repository_url}." + comment = f"🔴 Cannot extract metadata from the `paper.md` front-matter or the `myst.yml` for {repository_url}." gh_create_comment(github_client,REVIEW_REPOSITORY,issue_id,comment) return make_response(jsonify(f"Problem with parsing paper.md for {repository_url}"),404) diff --git a/tests/test_myst_frontmatter.py b/tests/test_myst_frontmatter.py index f0e4bab..7902410 100644 --- a/tests/test_myst_frontmatter.py +++ b/tests/test_myst_frontmatter.py @@ -1,6 +1,110 @@ import pytest from api.myst_frontmatter import myst_project_metadata +from api.myst_frontmatter import merge_paper_metadata + +FRONT_MATTER_PAPER = """--- +title: Front Matter Title +authors: + - name: Ada Lovelace + affiliation: "1" +affiliations: + - name: Analytical Engine Institute + index: 1 +--- + +Body. +""" + +AUTHORS_ONLY_PAPER = """--- +authors: + - name: Ada Lovelace + affiliation: "1" +--- + +Body. +""" + +MYST_YML = """project: + title: Myst Title + date: "02 February 2022" + keywords: + - myst keyword + authors: + - name: Grace Hopper + affiliations: society + affiliations: + - id: society + institution: Royal Society +""" + +MALFORMED_MYST_YML = 'project:\n title: "unterminated\n authors: [ {\n' + + +def test_front_matter_wins_over_myst_yml(): + data = merge_paper_metadata( + {"title": "Front Matter Title", + "authors": [{"name": "Ada Lovelace", "affiliation": "1"}], + "affiliations": [{"index": 1, "name": "Analytical Engine Institute"}]}, + MYST_YML, + ) + assert data["title"] == "Front Matter Title" + assert [a["name"] for a in data["authors"]] == ["Ada Lovelace"] + assert data["affiliations"] == [{"index": 1, "name": "Analytical Engine Institute"}] + + +def test_myst_yml_fills_a_paper_with_no_front_matter(): + data = merge_paper_metadata(None, MYST_YML) + assert data["title"] == "Myst Title" + assert data["authors"][0]["name"] == "Grace Hopper" + assert data["authors"][0]["affiliation"] == "1" + assert data["affiliations"] == [{"index": 1, "name": "Royal Society"}] + + +def test_scalar_fields_fill_individually(): + data = merge_paper_metadata({"title": "Kept"}, MYST_YML) + assert data["title"] == "Kept" + assert data["date"] == "02 February 2022" + assert data["tags"] == ["myst keyword"] + + +def test_authors_and_affiliations_are_filled_as_a_pair(): + # The front matter has authors but no affiliations, so both must come from + # myst.yml rather than pairing index 1 with the wrong institution. + data = merge_paper_metadata( + {"authors": [{"name": "Ada Lovelace", "affiliation": "1"}]}, MYST_YML + ) + assert [a["name"] for a in data["authors"]] == ["Grace Hopper"] + assert data["affiliations"] == [{"index": 1, "name": "Royal Society"}] + + +def test_returns_none_when_neither_source_has_authors(): + assert merge_paper_metadata(None, None) is None + assert merge_paper_metadata({"title": "Only a title"}, None) is None + + +def test_tolerates_a_malformed_myst_yml(): + data = merge_paper_metadata( + {"title": "Front Matter Title", + "authors": [{"name": "Ada Lovelace", "affiliation": "1"}], + "affiliations": [{"index": 1, "name": "Analytical Engine Institute"}]}, + MALFORMED_MYST_YML, + ) + assert data["title"] == "Front Matter Title" + assert [a["name"] for a in data["authors"]] == ["Ada Lovelace"] + + +def test_tolerates_a_myst_yml_with_no_project_key(): + data = merge_paper_metadata( + {"authors": [{"name": "Ada Lovelace"}]}, "site:\n title: Not a project\n" + ) + assert [a["name"] for a in data["authors"]] == ["Ada Lovelace"] + + +def test_does_not_mutate_the_caller_s_front_matter(): + front_matter = {"authors": [{"name": "Ada Lovelace", "affiliation": "1"}]} + merge_paper_metadata(front_matter, MYST_YML) + assert front_matter == {"authors": [{"name": "Ada Lovelace", "affiliation": "1"}]} def test_composes_affiliation_name_from_parts_in_order(): From 82a4699555ae44f1b7bc7086c8fbbc98691a9f26 Mon Sep 17 00:00:00 2001 From: Agah Date: Sun, 2 Aug 2026 22:31:17 -0400 Subject: [PATCH 03/11] Tolerate authors with no resolvable affiliation in the Zenodo deposit task --- api/myst_frontmatter.py | 34 +++++++++++++++++++++++++++++++ api/neurolibre_celery_tasks.py | 16 ++++++--------- tests/test_myst_frontmatter.py | 37 ++++++++++++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 10 deletions(-) diff --git a/api/myst_frontmatter.py b/api/myst_frontmatter.py index 9554d79..958b50d 100644 --- a/api/myst_frontmatter.py +++ b/api/myst_frontmatter.py @@ -169,3 +169,37 @@ def merge_paper_metadata(front_matter, myst_text): if not metadata.get("authors"): return None return metadata + + +def first_affiliations(authors, affiliations): + """Resolve each author's first affiliation to a display name. + + `authors` is a list of author dicts as produced by `merge_paper_metadata` + (or a hand-written paper.md front matter); each may carry an `affiliation` + value that is an int, a comma-separated string of indices, an empty + string, or absent entirely. `affiliations` is the corresponding list of + `{"index": ..., "name": ...}` mappings. + + Returns a list the same length as `authors`. An element is `None` when the + author has no affiliation, or names an index the affiliation list does not + define -- both are legitimate, not errors: myst.yml permits an author with + no affiliation (see `test_author_without_affiliations_gets_no_affiliation_key`), + and a caller should not have the deposit fail just because one author + lacks one. + """ + mapping = {str(affiliation["index"]): affiliation["name"] for affiliation in affiliations} + + resolved = [] + for author in authors: + affiliation = author.get("affiliation") + if not affiliation: + resolved.append(None) + continue + if isinstance(affiliation, int): + affiliation_index = affiliation + else: + affiliation_indices = [affiliation_index for affiliation_index in str(affiliation).split(",")] + affiliation_index = affiliation_indices[0] + resolved.append(mapping.get(str(affiliation_index))) + + return resolved diff --git a/api/neurolibre_celery_tasks.py b/api/neurolibre_celery_tasks.py index 598f704..732db8d 100644 --- a/api/neurolibre_celery_tasks.py +++ b/api/neurolibre_celery_tasks.py @@ -7,6 +7,7 @@ from celery import states from github_client import * from screening_client import ScreeningClient +from myst_frontmatter import first_affiliations from common import * from preprint import * from github import Github, UnknownObjectException, GithubException @@ -991,18 +992,13 @@ def zenodo_create_buckets_task(self, payload): # We need to go through some affiliation mapping here. affiliation_mapping = {str(affiliation['index']): affiliation['name'] for affiliation in data['affiliations']} - first_affiliations = [] - for author in data['authors']: - if isinstance(author['affiliation'],int): - affiliation_index = author['affiliation'] - else: - affiliation_indices = [affiliation_index for affiliation_index in author['affiliation'].split(',')] - affiliation_index = affiliation_indices[0] - first_affiliation = affiliation_mapping[str(affiliation_index)] - first_affiliations.append(first_affiliation) + resolved_affiliations = first_affiliations(data['authors'], data['affiliations']) for ii in range(len(data['authors'])): - data['authors'][ii]['affiliation'] = first_affiliations[ii] + if resolved_affiliations[ii] is None: + data['authors'][ii].pop('affiliation', None) + else: + data['authors'][ii]['affiliation'] = resolved_affiliations[ii] # To deal with some typos, also with orchid :) valid_field_names = {'name', 'orcid', 'affiliation'} diff --git a/tests/test_myst_frontmatter.py b/tests/test_myst_frontmatter.py index 7902410..e03966f 100644 --- a/tests/test_myst_frontmatter.py +++ b/tests/test_myst_frontmatter.py @@ -2,6 +2,7 @@ from api.myst_frontmatter import myst_project_metadata from api.myst_frontmatter import merge_paper_metadata +from api.myst_frontmatter import first_affiliations FRONT_MATTER_PAPER = """--- title: Front Matter Title @@ -215,3 +216,39 @@ def test_author_without_affiliations_gets_no_affiliation_key(): @pytest.mark.parametrize("project", [None, {}, "not a mapping", []]) def test_tolerates_junk_input(project): assert myst_project_metadata(project) == {} + + +AFFILIATIONS = [ + {"index": 1, "name": "Analytical Engine Institute"}, + {"index": 2, "name": "Royal Society"}, +] + + +def test_first_affiliations_resolves_a_single_index(): + authors = [{"name": "Ada Lovelace", "affiliation": "1"}] + assert first_affiliations(authors, AFFILIATIONS) == ["Analytical Engine Institute"] + + +def test_first_affiliations_takes_the_first_of_a_comma_string(): + authors = [{"name": "Grace Hopper", "affiliation": "2,1"}] + assert first_affiliations(authors, AFFILIATIONS) == ["Royal Society"] + + +def test_first_affiliations_accepts_an_int(): + authors = [{"name": "Ada Lovelace", "affiliation": 2}] + assert first_affiliations(authors, AFFILIATIONS) == ["Royal Society"] + + +def test_first_affiliations_is_none_when_the_key_is_absent(): + authors = [{"name": "The Analytical Collaboration"}] + assert first_affiliations(authors, AFFILIATIONS) == [None] + + +def test_first_affiliations_is_none_for_an_empty_string(): + authors = [{"name": "The Analytical Collaboration", "affiliation": ""}] + assert first_affiliations(authors, AFFILIATIONS) == [None] + + +def test_first_affiliations_is_none_for_an_undeclared_index(): + authors = [{"name": "Ada Lovelace", "affiliation": "9"}] + assert first_affiliations(authors, AFFILIATIONS) == [None] From b11a753e3581671d0ebe12cd72ef2d728249f0d6 Mon Sep 17 00:00:00 2001 From: Agah Date: Sun, 2 Aug 2026 23:14:21 -0400 Subject: [PATCH 04/11] Stringify myst.yml dates, honour empty front-matter keys, guard affiliations An unquoted `date: 2024-01-15` in myst.yml -- the MyST-canonical form -- is parsed by yaml.safe_load into a datetime.date, copied into the paper metadata, and handed to a Celery task whose payload is serialized as JSON. That raised inside a Flask route with no handler: HTTP 500, no GitHub comment, no buckets. Map a non-string date to its string form. A front-matter key that is present but empty (None, "", [], {}) now counts as absent, for both the scalar fills and the authors/affiliations pair check, so a paper.md that merely lists the keys no longer defeats the fallback. A bare string is accepted where MyST accepts one: an author entry becomes a named author with no affiliations, an affiliation entry becomes an affiliation named after the string with no id, and either still holds its index position -- matching inara's Lua filter, so a PDF and a Zenodo record cannot credit different institutions. A scalar `affiliations:` value is one entry rather than one per character. In the deposit task, drop the dead affiliation_mapping line that indexed data['affiliations'] unguarded, and pass data.get('affiliations') or [] so authors-without-affiliations metadata resolves every author to None instead of raising KeyError. An affiliation index the list does not define now logs a warning naming the author rather than silently recording no institution. Claude-Session: https://claude.ai/code/session_01YFCLUc1iAr5jjjuUTMMEB8 --- api/myst_frontmatter.py | 87 ++++++++++++++++++++++++++++----- api/neurolibre_celery_tasks.py | 7 +-- tests/test_myst_frontmatter.py | 89 ++++++++++++++++++++++++++++++++++ 3 files changed, 168 insertions(+), 15 deletions(-) diff --git a/api/myst_frontmatter.py b/api/myst_frontmatter.py index 958b50d..671fa5d 100644 --- a/api/myst_frontmatter.py +++ b/api/myst_frontmatter.py @@ -34,6 +34,36 @@ ALIASES = {"institution": "name", "region": "state"} +def _is_blank(value): + """Is a value absent, or present but carrying nothing? + + A `paper.md` front matter of `title:` parses to `title: None`, not to a + missing key. `""`, `[]` and `{}` say the same thing. All of them must count + as absent or a key that was merely typed out defeats the myst.yml fallback. + Mirrors `is_blank` in inara's myst-frontmatter.lua. + """ + if value is None: + return True + if isinstance(value, str): + return not value.strip() + if isinstance(value, (list, tuple, dict, set)): + return len(value) == 0 + return False + + +def _as_list(value): + """Normalise a myst.yml sequence to a list. + + `affiliations: harvard` is legal MyST. Without this, iterating the string + would walk its characters. Mirrors `as_list` in myst-frontmatter.lua. + """ + if value is None: + return [] + if isinstance(value, (list, tuple)): + return list(value) + return [value] + + def _affiliation_name(affiliation): """Join an affiliation's parts into a single display string.""" parts = [] @@ -71,28 +101,44 @@ def myst_project_metadata(project): metadata = {} - if project.get("title") is not None: + if not _is_blank(project.get("title")): metadata["title"] = project["title"] - if project.get("date") is not None: - metadata["date"] = project["date"] - if project.get("keywords") is not None: + if not _is_blank(project.get("date")): + # `date: 2024-01-15` -- unquoted ISO, the MyST-canonical form -- is + # parsed by yaml.safe_load into a datetime.date. This value is carried + # into a Celery task payload, which is serialized as JSON, so a date + # object here is an HTTP 500 at enqueue time. Nothing downstream + # consumes `date` structurally, so the string form is the right shape. + date = project["date"] + metadata["date"] = date if isinstance(date, str) else str(date) + if not _is_blank(project.get("keywords")): metadata["tags"] = project["keywords"] - if project.get("bibliography") is not None: + if not _is_blank(project.get("bibliography")): metadata["bibliography"] = project["bibliography"] affiliations = [] index_of = {} - for source in project.get("affiliations") or []: + for source in _as_list(project.get("affiliations")): + index = len(affiliations) + 1 if not isinstance(source, dict): + # MyST's validator accepts a bare string where an affiliation + # mapping is expected. It becomes an affiliation named after that + # string, with no id, and it still consumes its index position -- + # the Lua filter applies the same rule, so both sides agree on + # every author's index. + affiliations.append({"index": index, "name": str(source).strip()}) continue - index = len(affiliations) + 1 affiliations.append({"index": index, "name": _affiliation_name(source)}) if source.get("id") is not None: index_of[str(source["id"])] = index authors = [] - for source in project.get("authors") or []: + for source in _as_list(project.get("authors")): if not isinstance(source, dict): + # Same MyST rule for authors: `authors: [Ada Lovelace]` is valid. + # A bare string becomes a named author with no affiliations, still + # holding its position in the list. + authors.append({"name": str(source).strip()}) continue author = {"name": source.get("name")} for target, key in ( @@ -158,12 +204,14 @@ def merge_paper_metadata(front_matter, myst_text): # only means something relative to the list that defines it, so mixing # the two sources would silently attach authors to the wrong # institutions. - if "authors" not in metadata or "affiliations" not in metadata: + # + # A key that is present but empty counts as absent -- see `_is_blank`. + if _is_blank(metadata.get("authors")) or _is_blank(metadata.get("affiliations")): if fallback.get("authors"): metadata["authors"] = fallback["authors"] metadata["affiliations"] = fallback.get("affiliations", []) for key in ("title", "date", "tags", "bibliography"): - if key not in metadata and key in fallback: + if _is_blank(metadata.get(key)) and key in fallback: metadata[key] = fallback[key] if not metadata.get("authors"): @@ -186,8 +234,14 @@ def first_affiliations(authors, affiliations): no affiliation (see `test_author_without_affiliations_gets_no_affiliation_key`), and a caller should not have the deposit fail just because one author lacks one. + + An empty `affiliations` list is legitimate too -- a myst.yml project may name + authors and no institutions at all -- and resolves every author to `None`. """ - mapping = {str(affiliation["index"]): affiliation["name"] for affiliation in affiliations} + mapping = { + str(affiliation["index"]): affiliation["name"] + for affiliation in affiliations or [] + } resolved = [] for author in authors: @@ -200,6 +254,15 @@ def first_affiliations(authors, affiliations): else: affiliation_indices = [affiliation_index for affiliation_index in str(affiliation).split(",")] affiliation_index = affiliation_indices[0] - resolved.append(mapping.get(str(affiliation_index))) + name = mapping.get(str(affiliation_index)) + if name is None: + # A typo'd index used to crash loudly; now it silently records a + # creator with no institution. Say so, so it is diagnosable. + logging.warning( + f"Affiliation index {affiliation_index!r} for author " + f"{author.get('name')!r} is not defined by the affiliation " + f"list; recording no affiliation for this author." + ) + resolved.append(name) return resolved diff --git a/api/neurolibre_celery_tasks.py b/api/neurolibre_celery_tasks.py index 732db8d..b2018f6 100644 --- a/api/neurolibre_celery_tasks.py +++ b/api/neurolibre_celery_tasks.py @@ -990,9 +990,10 @@ def zenodo_create_buckets_task(self, payload): data = payload['paper_data'] - # We need to go through some affiliation mapping here. - affiliation_mapping = {str(affiliation['index']): affiliation['name'] for affiliation in data['affiliations']} - resolved_affiliations = first_affiliations(data['authors'], data['affiliations']) + # We need to go through some affiliation mapping here. The affiliation list + # can be absent entirely -- authors in the front matter plus a myst.yml + # project that names none -- so do not index it directly. + resolved_affiliations = first_affiliations(data['authors'], data.get('affiliations') or []) for ii in range(len(data['authors'])): if resolved_affiliations[ii] is None: diff --git a/tests/test_myst_frontmatter.py b/tests/test_myst_frontmatter.py index e03966f..488b903 100644 --- a/tests/test_myst_frontmatter.py +++ b/tests/test_myst_frontmatter.py @@ -1,3 +1,5 @@ +import json + import pytest from api.myst_frontmatter import myst_project_metadata @@ -252,3 +254,90 @@ def test_first_affiliations_is_none_for_an_empty_string(): def test_first_affiliations_is_none_for_an_undeclared_index(): authors = [{"name": "Ada Lovelace", "affiliation": "9"}] assert first_affiliations(authors, AFFILIATIONS) == [None] + + +def test_first_affiliations_warns_about_an_undeclared_index(caplog): + authors = [{"name": "Ada Lovelace", "affiliation": "9"}] + with caplog.at_level("WARNING"): + assert first_affiliations(authors, AFFILIATIONS) == [None] + assert "Ada Lovelace" in caplog.text + assert "9" in caplog.text + + +def test_first_affiliations_tolerates_an_empty_affiliation_list(): + # A front matter with authors only, plus a myst.yml project that names no + # authors, reaches the deposit task with authors and no affiliations. + authors = [{"name": "Ada Lovelace", "affiliation": "1"}, {"name": "Grace Hopper"}] + assert first_affiliations(authors, []) == [None, None] + + +UNQUOTED_DATE_MYST_YML = """project: + date: 2024-01-15 + authors: + - name: Grace Hopper +""" + + +def test_unquoted_iso_date_maps_to_a_string(): + # yaml.safe_load turns an unquoted ISO date into a datetime.date. + data = merge_paper_metadata(None, UNQUOTED_DATE_MYST_YML) + assert data["date"] == "2024-01-15" + assert isinstance(data["date"], str) + + +def test_metadata_from_an_unquoted_date_is_json_serialisable(): + # The real failure mode: the metadata becomes a Celery task payload, and + # Celery serialises tasks as JSON. + data = merge_paper_metadata(None, UNQUOTED_DATE_MYST_YML) + assert json.loads(json.dumps(data))["date"] == "2024-01-15" + + +@pytest.mark.parametrize("blank", [None, "", [], {}]) +def test_blank_front_matter_keys_are_filled_from_myst_yml(blank): + data = merge_paper_metadata( + {"title": blank, "authors": blank, "affiliations": blank}, MYST_YML + ) + assert data["title"] == "Myst Title" + assert [a["name"] for a in data["authors"]] == ["Grace Hopper"] + assert data["affiliations"] == [{"index": 1, "name": "Royal Society"}] + + +def test_bare_string_authors_become_named_authors(): + project = {"authors": ["Ada Lovelace", "Grace Hopper"]} + authors = myst_project_metadata(project)["authors"] + assert [a["name"] for a in authors] == ["Ada Lovelace", "Grace Hopper"] + assert all("affiliation" not in author for author in authors) + + +MIXED_AFFILIATIONS_PROJECT = { + "authors": [{"name": "Ada Lovelace", "affiliations": "b"}], + "affiliations": [ + {"id": "a", "institution": "Alpha University"}, + "Bare String Institute", + {"id": "b", "institution": "Beta University"}, + ], +} + + +def test_a_bare_string_affiliation_consumes_its_index_position(): + result = myst_project_metadata(MIXED_AFFILIATIONS_PROJECT) + assert result["affiliations"] == [ + {"index": 1, "name": "Alpha University"}, + {"index": 2, "name": "Bare String Institute"}, + {"index": 3, "name": "Beta University"}, + ] + assert result["authors"][0]["affiliation"] == "3" + + +def test_a_scalar_affiliations_value_is_one_affiliation(): + # `affiliations: harvard` is legal MyST; iterating the string would yield + # one affiliation per character. + result = myst_project_metadata( + {"authors": [{"name": "Ada Lovelace"}], "affiliations": "Harvard University"} + ) + assert result["affiliations"] == [{"index": 1, "name": "Harvard University"}] + + +def test_a_scalar_authors_value_is_one_author(): + result = myst_project_metadata({"authors": "Ada Lovelace"}) + assert [a["name"] for a in result["authors"]] == ["Ada Lovelace"] From 05d4fa20ddd5f8098506612d97c7b6d27e61402f Mon Sep 17 00:00:00 2001 From: Agah Date: Tue, 4 Aug 2026 00:10:22 -0400 Subject: [PATCH 05/11] Remove dead get_active_ports helper Uncalled, and its 3001-3099 range never covered the content server's 3100-3200. Claude-Session: https://claude.ai/code/session_016qxMAGM5tpB5okf7hJJgJW --- api/common.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/api/common.py b/api/common.py index 495e2fa..5fced84 100644 --- a/api/common.py +++ b/api/common.py @@ -570,13 +570,6 @@ def run_celery_subprocess(command, log_output=True): logging.error(f"Command: {' '.join(command)}") return -1, str(e) -def get_active_ports(start=3001, end=3099): - active_ports = [] - for conn in psutil.net_connections(kind='inet'): - if conn.status == psutil.CONN_LISTEN and start <= conn.laddr.port <= end: - active_ports.append(conn.laddr.port) - return active_ports - def close_port_by_pid(target_pid): """Kill the entire process group rooted at target_pid. From 0b318680e3a3012312837b096b16de81c3850e4c Mon Sep 17 00:00:00 2001 From: Agah Date: Tue, 4 Aug 2026 00:23:28 -0400 Subject: [PATCH 06/11] Reap orphaned myst process groups on worker startup Hooks celeryd_after_setup so it runs once per node, before tasks are consumed. Claude-Session: https://claude.ai/code/session_016qxMAGM5tpB5okf7hJJgJW --- api/neurolibre_celery_tasks.py | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/api/neurolibre_celery_tasks.py b/api/neurolibre_celery_tasks.py index b2018f6..6adbd08 100644 --- a/api/neurolibre_celery_tasks.py +++ b/api/neurolibre_celery_tasks.py @@ -1,4 +1,5 @@ from celery import Celery +from celery.signals import celeryd_after_setup import time import os import json @@ -23,6 +24,7 @@ from myst_libre.tools import JupyterHubLocalSpawner from myst_libre.rees import REES from myst_libre.builders import MystBuilder +from myst_libre.tools import MystMD from celery.schedules import crontab import zipfile import tempfile @@ -120,6 +122,34 @@ # DB 0 is the Celery broker; we use DB 2 for locks to avoid key collisions. _lock_redis = redis_lib.Redis(host='localhost', port=6379, db=2) + +@celeryd_after_setup.connect +def reap_myst_orphans(sender, instance, **kwargs): + """ + Clean up myst process groups left behind by a previous worker. + + A crashed or restarted worker leaves myst and its children (npm run start -> + node ./server.js) running and holding ports. Normal teardown kills the + process group, but that needs a live PID to signal. + + celeryd_after_setup fires once in the main worker process, after setup and + before children fork or any task is consumed. worker_process_init would be + wrong here: it runs in every prefork child, so N reaps would race. + + Builds running in sibling workers are unaffected - myst-libre only reaps + records whose owning process is gone. + """ + try: + reaped = MystMD.reap_orphans() + if reaped: + logging.warning( + f"Reaped {len(reaped)} orphaned myst process group(s): " + f"{[e.get('build_dir') for e in reaped]}" + ) + except Exception as e: + # Never block worker startup over cleanup + logging.warning(f"Orphan reaping failed: {e}") + """ Configuration END """ From e7f482a386110ce2fd215c95e1f90052709d8c1a Mon Sep 17 00:00:00 2001 From: Agah Date: Tue, 4 Aug 2026 10:39:36 -0400 Subject: [PATCH 07/11] Add systemd unit blocking instance metadata for build containers Re-adds the DOCKER-USER rule after docker.service on every boot; idempotent so restarts cannot stack duplicates. Claude-Session: https://claude.ai/code/session_016qxMAGM5tpB5okf7hJJgJW --- README.md | 33 +++++++++++++++++++ about.md | 1 + systemd/neurolibre-mystbuild-firewall.service | 30 +++++++++++++++++ 3 files changed, 64 insertions(+) create mode 100644 systemd/neurolibre-mystbuild-firewall.service diff --git a/README.md b/README.md index 40cde1a..3eafaf8 100644 --- a/README.md +++ b/README.md @@ -215,6 +215,39 @@ This should start multiple `gunicorn` workers, each one of them binding our flas > Reminder: Replace the **``** in the commands above either with `preprint` or `preview` depending on the server (e.g., `neurolibre-preview.service`) you are configuring. Note that this is not only a naming convention, but also defines a functional separation between the roles of the two servers. +#### Isolate MyST build containers from instance metadata + +Build containers execute notebook code from submitted repositories. On the default Docker bridge they can reach the instance metadata service (`169.254.169.254` on OpenStack), which serves user-data and injected credentials. + +Create a dedicated network with a fixed bridge name, so the firewall rule has something stable to match: + +``` +docker network create --driver bridge \ + --opt com.docker.network.bridge.name=br-mystbuild mystbuild +docker pull busybox:latest +``` + +Install the service that blocks metadata for that bridge on every boot: + +``` +sudo cp ~/full-stack-server/systemd/neurolibre-mystbuild-firewall.service /etc/systemd/system/ +sudo systemctl daemon-reload +sudo systemctl enable --now neurolibre-mystbuild-firewall.service +``` + +Verify — this is the only thing that proves the rule is working: + +``` +docker run --rm --network mystbuild curlimages/curl \ + -s -m 3 http://169.254.169.254/openstack/ ; echo "exit=$?" +``` + +A non-zero exit (`7` rejected, `28` timeout) means blocked. `exit=0` with a listing of API versions means it is **not** blocked — check that `br-mystbuild` exists (`ip -o link show br-mystbuild`) and that metadata is not a local address (`ip addr | grep 169.254`, which should print nothing). + +Then pass `container_network = 'mystbuild'` to `JupyterHubLocalSpawner` in `api/neurolibre_celery_tasks.py` and restart the Celery worker. myst-libre re-checks this before every build session and refuses to spawn if metadata answers, so a rule lost after a reboot fails loudly instead of silently reopening. + +> Do not use `iptables-persistent` for this rule. It snapshots the entire ruleset including Docker's generated rules, and restoring those at boot before Docker starts causes duplicated and conflicting rules. The systemd unit is ordered after `docker.service` and re-adds only this rule. + #### Configure Celery as a systemd service For Celery async task queue manager to work, there are two requirements: diff --git a/about.md b/about.md index 65db558..23eb03a 100644 --- a/about.md +++ b/about.md @@ -87,6 +87,7 @@ celery -A neurolibre_celery_tasks worker --loglevel=info The application runs as systemd services: - `neurolibre-preview.service` - Preview server - `neurolibre-preprint.service` - Preprint server +- `neurolibre-mystbuild-firewall.service` - Blocks instance metadata for build containers - Celery workers for async tasks ## Key Directories diff --git a/systemd/neurolibre-mystbuild-firewall.service b/systemd/neurolibre-mystbuild-firewall.service new file mode 100644 index 0000000..811d53d --- /dev/null +++ b/systemd/neurolibre-mystbuild-firewall.service @@ -0,0 +1,30 @@ +[Unit] +# Blocks the OpenStack instance metadata service (169.254.169.254) for +# containers on the myst build network. Build containers execute notebook code +# from submitted repositories; metadata serves user-data and injected +# credentials, so it must not be reachable from them. +# +# Docker has no per-container egress ACL, hence a host rule. DOCKER-USER is +# processed before Docker's own chains and matches forwarded traffic only, so +# the host's own metadata access (cloud-init) is unaffected. +# +# Requires the build network to exist with a fixed bridge name: +# docker network create --driver bridge \ +# --opt com.docker.network.bridge.name=br-mystbuild mystbuild +# +# Do NOT use iptables-persistent for this. It snapshots the entire ruleset, +# including Docker's generated rules, and restoring those at boot before Docker +# starts causes duplicated and conflicting rules. +Description=Block instance metadata for myst build containers +After=docker.service +Requires=docker.service + +[Service] +Type=oneshot +RemainAfterExit=yes +# -C tests for the rule first, so restarting the unit cannot stack duplicates +ExecStart=/bin/sh -c '/sbin/iptables -C DOCKER-USER -i br-mystbuild -d 169.254.0.0/16 -j REJECT 2>/dev/null || /sbin/iptables -I DOCKER-USER -i br-mystbuild -d 169.254.0.0/16 -j REJECT' +ExecStop=/bin/sh -c '/sbin/iptables -D DOCKER-USER -i br-mystbuild -d 169.254.0.0/16 -j REJECT 2>/dev/null || true' + +[Install] +WantedBy=multi-user.target From 0dc237a8ef0073d8a33a3fa4252e331601026696 Mon Sep 17 00:00:00 2001 From: Agah Date: Tue, 4 Aug 2026 12:14:32 -0400 Subject: [PATCH 08/11] Guard each cleanup step so a failure cannot leak the build lock An exception in builder.cleanup() aborted the whole finally block, leaving the container running and the repo locked until the 6000s timeout. Claude-Session: https://claude.ai/code/session_016qxMAGM5tpB5okf7hJJgJW --- api/neurolibre_celery_tasks.py | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/api/neurolibre_celery_tasks.py b/api/neurolibre_celery_tasks.py index 6adbd08..2b120c2 100644 --- a/api/neurolibre_celery_tasks.py +++ b/api/neurolibre_celery_tasks.py @@ -2065,16 +2065,28 @@ def preview_build_myst_task(self, screening_dict): # Always clean up the myst process tree (kills the entire process # group: myst node + npm run start + node ./server.js) and the # JupyterHub container, regardless of success or failure. + # + # Each step is guarded independently. Previously an exception in the + # first one aborted the rest of this block, leaking the container AND + # the build lock - which then blocks every build of that repo until the + # 6000s timeout expires. A failed cleanup step must not cost more than + # itself. if builder is not None: - builder.cleanup() - cleanup_hub(hub) + try: + builder.cleanup() + except Exception as e: + logging.warning(f"builder.cleanup() failed: {e}") + try: + cleanup_hub(hub) + except Exception as e: + logging.warning(f"cleanup_hub() failed: {e}") try: build_lock.release() except redis_lib.exceptions.LockNotOwnedError: # Lock expired (build exceeded timeout) and was auto-released. logging.warning(f"Build lock {lock_key} already expired.") - except Exception: - pass + except Exception as e: + logging.warning(f"Could not release build lock {lock_key}: {e}") @celery_app.task(bind=True) @handle_soft_timeout From b66e993a620d2d35afc669ef5bc711af0234d57c Mon Sep 17 00:00:00 2001 From: Agah Date: Wed, 19 Aug 2026 18:27:57 -0400 Subject: [PATCH 09/11] Deposit only the author fields Zenodo accepts The myst.yml fallback made author metadata richer than the deposit path can use. A myst.yml author routinely carries email, github, twitter, url and corresponding; Zenodo's legacy schema allows a creator only name, affiliation, orcid and gnd. The sanitizer in zenodo_create_buckets_task popped corresponding and equal-contrib by name but had no case for email, and its substring repair could not invent one -- no valid field name is a substring of "email" -- so it reached metadata.creators. That risks a validation error on the deposit, and publishes nine author email addresses on a public record if it does not. So the deposit boundary now decides what a creator is, in zenodo_metadata, applied inside zenodo_create_bucket rather than in its caller so every deposit path is covered. It keeps the typo repair the old loop did, and scans an ordered tuple to do it: the loop iterated a set, so a key matching more than one field mapped differently between runs. Also closes the gaps around it, all of which turn a bad submission into an error nobody sees: - api_zenodo_post checked only that metadata named authors, while the task also requires a title. Missing it raised KeyError inside Celery. - first_affiliations subscripted affiliation["index"]/["name"], so one malformed entry in a hand-written paper.md failed the whole deposit. A bare string author raised AttributeError for the same reason. - Replacing the front matter's author list with myst.yml's is correct -- an affiliation index only means something relative to its list -- but silent, so a stale myst.yml outranking a current paper.md was indistinguishable from a working fallback. It logs now. - The pending comment was passed the author list where a task id goes. Verified against the live haplante/oct-t1-paper, whose paper.md declares only a title and puts its authors in myst.yml: 9 authors in, 9 creators out, carrying name, orcid and affiliation alone. Claude-Session: https://claude.ai/code/session_016Tcpjta8yB3my2RLeH5tty --- api/myst_frontmatter.py | 42 +++++++++-- api/neurolibre_celery_tasks.py | 33 ++------- api/neurolibre_preprint_api.py | 13 +++- api/preprint.py | 5 +- api/zenodo_metadata.py | 115 ++++++++++++++++++++++++++++ tests/test_myst_frontmatter.py | 32 ++++++++ tests/test_zenodo_metadata.py | 132 +++++++++++++++++++++++++++++++++ 7 files changed, 337 insertions(+), 35 deletions(-) create mode 100644 api/zenodo_metadata.py create mode 100644 tests/test_zenodo_metadata.py diff --git a/api/myst_frontmatter.py b/api/myst_frontmatter.py index 671fa5d..4eff044 100644 --- a/api/myst_frontmatter.py +++ b/api/myst_frontmatter.py @@ -208,6 +208,17 @@ def merge_paper_metadata(front_matter, myst_text): # A key that is present but empty counts as absent -- see `_is_blank`. if _is_blank(metadata.get("authors")) or _is_blank(metadata.get("affiliations")): if fallback.get("authors"): + if not _is_blank(metadata.get("authors")): + # The front matter named authors but no affiliations, so its + # author list is discarded rather than merged. Announce it: + # a stale myst.yml silently outranking a current paper.md is + # otherwise indistinguishable from a correct fallback. + logging.warning( + "paper.md names authors but no affiliations; replacing " + "its author list with the one from myst.yml, because an " + "affiliation index only means something relative to the " + "list that defines it." + ) metadata["authors"] = fallback["authors"] metadata["affiliations"] = fallback.get("affiliations", []) for key in ("title", "date", "tags", "bibliography"): @@ -238,13 +249,30 @@ def first_affiliations(authors, affiliations): An empty `affiliations` list is legitimate too -- a myst.yml project may name authors and no institutions at all -- and resolves every author to `None`. """ - mapping = { - str(affiliation["index"]): affiliation["name"] - for affiliation in affiliations or [] - } + # Built with `.get`, not subscripting: a hand-written paper.md may omit + # `index` or `name` on one entry, and that entry alone should be unusable + # rather than raising and failing the deposit. + mapping = {} + for affiliation in affiliations or []: + if not isinstance(affiliation, dict): + continue + index = affiliation.get("index") + name = affiliation.get("name") + if index is None or name is None: + logging.warning( + f"Ignoring an affiliation entry missing 'index' or 'name': " + f"{affiliation!r}." + ) + continue + mapping[str(index).strip()] = name resolved = [] for author in authors: + if not isinstance(author, dict): + # `authors: [Ada Lovelace]` is legal in both sources; a bare string + # names no affiliation. + resolved.append(None) + continue affiliation = author.get("affiliation") if not affiliation: resolved.append(None) @@ -252,9 +280,9 @@ def first_affiliations(authors, affiliations): if isinstance(affiliation, int): affiliation_index = affiliation else: - affiliation_indices = [affiliation_index for affiliation_index in str(affiliation).split(",")] - affiliation_index = affiliation_indices[0] - name = mapping.get(str(affiliation_index)) + # `affiliation: "1, 2"` is as common as `"1,2"` in front matter. + affiliation_index = str(affiliation).split(",")[0].strip() + name = mapping.get(str(affiliation_index).strip()) if name is None: # A typo'd index used to crash loudly; now it silently records a # creator with no institution. Say so, so it is diagnosable. diff --git a/api/neurolibre_celery_tasks.py b/api/neurolibre_celery_tasks.py index 2b120c2..9eeaef3 100644 --- a/api/neurolibre_celery_tasks.py +++ b/api/neurolibre_celery_tasks.py @@ -1026,37 +1026,18 @@ def zenodo_create_buckets_task(self, payload): resolved_affiliations = first_affiliations(data['authors'], data.get('affiliations') or []) for ii in range(len(data['authors'])): + # A bare string author (`authors: [Ada Lovelace]`) is legal in both + # sources and carries no affiliation to resolve. + if not isinstance(data['authors'][ii], dict): + continue if resolved_affiliations[ii] is None: data['authors'][ii].pop('affiliation', None) else: data['authors'][ii]['affiliation'] = resolved_affiliations[ii] - # To deal with some typos, also with orchid :) - valid_field_names = {'name', 'orcid', 'affiliation'} - for author in data['authors']: - invalid_fields = [] - for field in author: - if field not in valid_field_names: - invalid_fields.append(field) - - for invalid_field in invalid_fields: - valid_field = None - for valid_name in valid_field_names: - if valid_name.lower() in invalid_field.lower() or (valid_name == 'orcid' and invalid_field.lower() == 'orchid'): - valid_field = valid_name - break - - if valid_field: - author[valid_field] = author.pop(invalid_field) - - if 'equal-contrib' in author: - author.pop('equal-contrib') - - if 'corresponding' in author: - author.pop('corresponding') - - # if author.get('orcid') is None: - # author.pop('orcid') + # Author fields are not filtered here: `zenodo_create_bucket` reduces them + # to the fields a Zenodo creator accepts (see `zenodo_metadata`), so the + # deposit boundary owns that rule and every caller gets it. collect = {} for archive_type in payload['archive_assets']: diff --git a/api/neurolibre_preprint_api.py b/api/neurolibre_preprint_api.py index b307eca..5823f5f 100644 --- a/api/neurolibre_preprint_api.py +++ b/api/neurolibre_preprint_api.py @@ -335,8 +335,19 @@ def api_zenodo_post(user,id,repository_url): gh_create_comment(github_client,REVIEW_REPOSITORY,issue_id,comment) return make_response(jsonify(f"Problem with parsing paper.md for {repository_url}"),404) + # `paper_data` is guaranteed to name authors and nothing else. The deposit + # needs a title too, so check it here: missing it deeper in the task means a + # KeyError inside Celery, which the author never sees. + if not paper_data.get('title'): + comment = f"🔴 Cannot determine the title of the submission from the `paper.md` front-matter or the `myst.yml` for {repository_url}." + gh_create_comment(github_client,REVIEW_REPOSITORY,issue_id,comment) + return make_response(jsonify(f"Missing title for {repository_url}"),404) + task_title = "Reproducibility Assets - Create Zenodo buckets" - comment_id = gh_template_respond(github_client,"pending",task_title,REVIEW_REPOSITORY,issue_id,paper_data['authors']) + # No task id yet -- one is stamped onto this comment by the "received" phase + # below. It used to be passed the author list, which rendered it as the task + # id in the pending comment. + comment_id = gh_template_respond(github_client,"pending",task_title,REVIEW_REPOSITORY,issue_id) celery_payload = dict(task_title = task_title, issue_id= issue_id, diff --git a/api/preprint.py b/api/preprint.py index e5c61da..a1b2822 100644 --- a/api/preprint.py +++ b/api/preprint.py @@ -7,6 +7,7 @@ import re from github import Github from github_client import gh_read_from_issue_body +from zenodo_metadata import zenodo_creators import csv import subprocess import nbformat @@ -76,7 +77,9 @@ def zenodo_create_bucket(title, archive_type, creators, repository_url, issue_id data = {} data["metadata"] = {} data["metadata"]["title"] = f"({tmp_type}) {title}" - data["metadata"]["creators"] = creators + # Whatever the submission declared, a creator is only what Zenodo accepts. + # Enforced here rather than in the caller so every deposit path is covered. + data["metadata"]["creators"] = zenodo_creators(creators) data["metadata"]["keywords"] = ["canadian-open-neuroscience-platform","neurolibre"] # (A) NeuroLibre artifact is a part of (isPartOf) the NeuroLibre preprint (B 10.55458/NeuroLibre.issue_id) data["metadata"]["related_identifiers"] = [{"relation": "isPartOf","identifier": f"{DOI_PREFIX}/{DOI_SUFFIX}.{issue_id:05d}","resource_type": "publication-preprint"}] diff --git a/api/zenodo_metadata.py b/api/zenodo_metadata.py new file mode 100644 index 0000000..8c9a1e1 --- /dev/null +++ b/api/zenodo_metadata.py @@ -0,0 +1,115 @@ +"""Reduce paper metadata to the shape a Zenodo deposit accepts. + +Paper metadata reaches the deposit path from two sources -- the paper.md front +matter and, filling its gaps, the myst.yml project (see `myst_frontmatter`). +Both describe authors more richly than Zenodo's legacy deposit schema allows a +creator to be: myst.yml authors routinely carry `email`, `github`, `twitter`, +`url` and `corresponding`, none of which Zenodo's `creators` accepts. Sending +them risks a validation error on the deposit, and an email address in +particular would be published on a public record. + +So the deposit boundary decides what a creator is, rather than trusting +whatever the submission happened to declare. This module is pure; the caller +fetches and merges. +""" + +import logging + +# The legacy Zenodo deposit schema for one entry of `metadata.creators`. +# Ordered, because a misspelled key is repaired by scanning this sequence and +# the first match wins -- iterating a set here made the repair depend on hash +# order, so the same author could map differently between runs. +ZENODO_CREATOR_FIELDS = ("name", "affiliation", "orcid", "gnd") + +# Misspellings seen in submissions that substring matching cannot repair. +CREATOR_FIELD_TYPOS = {"orchid": "orcid"} + + +def _is_blank(value): + """Is a value absent, or present but carrying nothing? + + Mirrors `myst_frontmatter._is_blank`: a key that was typed out but left + empty must not reach Zenodo as an empty creator field. + """ + if value is None: + return True + if isinstance(value, str): + return not value.strip() + if isinstance(value, (list, tuple, dict, set)): + return len(value) == 0 + return False + + +def _canonical_creator_field(key): + """Which Zenodo creator field does an author key mean, if any? + + Returns None for a key with no Zenodo counterpart -- `email` and friends -- + which is how they get dropped. + """ + lowered = str(key).strip().lower() + if lowered in ZENODO_CREATOR_FIELDS: + return lowered + if lowered in CREATOR_FIELD_TYPOS: + return CREATOR_FIELD_TYPOS[lowered] + for field in ZENODO_CREATOR_FIELDS: + # Catches `affiliations`, `author name`, and similar near misses. + if field in lowered: + return field + return None + + +def zenodo_creators(authors): + """Return `authors` as Zenodo creators, carrying only accepted fields. + + `authors` is a list of author mappings as produced by `merge_paper_metadata` + (or written by hand in a paper.md front matter), with affiliations already + resolved to display names by `first_affiliations`. A bare string is accepted + where a mapping is expected, matching what MyST permits. + + An author with no name is dropped, with a warning: Zenodo requires a name + on every creator, so including one would fail the entire deposit rather + than lose the one entry. Junk input yields an empty list -- this function + must never itself be the reason a deposit fails. + + The caller's authors are left untouched. + """ + if not isinstance(authors, (list, tuple)): + return [] + + creators = [] + for author in authors: + if not isinstance(author, dict): + if _is_blank(author): + continue + creators.append({"name": str(author).strip()}) + continue + + creator = {} + repaired = {} + for key, value in author.items(): + if _is_blank(value): + continue + field = _canonical_creator_field(key) + if field is None: + continue + target = creator if str(key).strip().lower() == field else repaired + target.setdefault(field, value) + + # An exactly-named key is authoritative; a repaired one only fills a + # field the author did not spell correctly anywhere. + for field, value in repaired.items(): + creator.setdefault(field, value) + + if not creator.get("name"): + logging.warning( + f"Skipping an author with no name in the Zenodo creator list: " + f"{author!r}." + ) + continue + + creators.append({ + field: value if isinstance(value, str) else str(value) + for field, value in creator.items() + }) + + return creators diff --git a/tests/test_myst_frontmatter.py b/tests/test_myst_frontmatter.py index 488b903..89035d3 100644 --- a/tests/test_myst_frontmatter.py +++ b/tests/test_myst_frontmatter.py @@ -341,3 +341,35 @@ def test_a_scalar_affiliations_value_is_one_affiliation(): def test_a_scalar_authors_value_is_one_author(): result = myst_project_metadata({"authors": "Ada Lovelace"}) assert [a["name"] for a in result["authors"]] == ["Ada Lovelace"] + + +def test_first_affiliations_tolerates_a_malformed_affiliation_entry(): + # A hand-written paper.md may omit `index` or `name` on an entry. That is a + # typo in one entry, not a reason to fail the whole deposit. + authors = [{"name": "Ada Lovelace", "affiliation": "1"}] + affiliations = [{"name": "No Index Institute"}, {"index": 1}] + assert first_affiliations(authors, affiliations) == [None] + + +def test_first_affiliations_strips_whitespace_around_an_index(): + authors = [{"name": "Ada Lovelace", "affiliation": " 1 , 2"}] + affiliations = [{"index": 1, "name": "Analytical Engine Institute"}] + assert first_affiliations(authors, affiliations) == [ + "Analytical Engine Institute" + ] + + +def test_first_affiliations_tolerates_a_bare_string_author(): + # `authors: [Ada Lovelace]` is legal in both sources. + assert first_affiliations(["Ada Lovelace"], []) == [None] + + +def test_warns_when_myst_yml_authors_replace_front_matter_authors(caplog): + # Authors and affiliations are filled as a pair, so a front matter that + # names authors but no affiliations loses its author list entirely. Say so. + with caplog.at_level("WARNING"): + metadata = merge_paper_metadata( + {"authors": [{"name": "Ada Lovelace"}]}, MYST_YML + ) + assert metadata["authors"][0]["name"] == "Grace Hopper" + assert "replacing" in caplog.text.lower() diff --git a/tests/test_zenodo_metadata.py b/tests/test_zenodo_metadata.py new file mode 100644 index 0000000..3d50034 --- /dev/null +++ b/tests/test_zenodo_metadata.py @@ -0,0 +1,132 @@ +"""What reaches a Zenodo deposit, and what must not.""" + +import json + +import pytest + +from api.zenodo_metadata import ZENODO_CREATOR_FIELDS +from api.zenodo_metadata import zenodo_creators + + +def test_keeps_only_the_fields_zenodo_accepts(): + creators = zenodo_creators([ + { + "name": "Ada Lovelace", + "orcid": "0000-0001-0000-0000", + "affiliation": "Analytical Engine Institute", + } + ]) + assert creators == [{ + "name": "Ada Lovelace", + "orcid": "0000-0001-0000-0000", + "affiliation": "Analytical Engine Institute", + }] + + +def test_drops_the_author_email(): + # myst.yml authors routinely carry an email; paper.md front matter rarely + # does. Zenodo rejects the key, and a public record must not publish it. + creators = zenodo_creators([ + {"name": "Ada Lovelace", "email": "ada@example.org"} + ]) + assert creators == [{"name": "Ada Lovelace"}] + + +def test_drops_the_myst_only_author_keys(): + creators = zenodo_creators([{ + "name": "Ada Lovelace", + "email": "ada@example.org", + "corresponding": True, + "equal-contrib": True, + "github": "ada", + "twitter": "ada", + "url": "https://example.org", + "numbering": {"heading_1": False}, + }]) + assert creators == [{"name": "Ada Lovelace"}] + + +def test_repairs_a_misspelled_orcid_key(): + creators = zenodo_creators([ + {"name": "Ada Lovelace", "orchid": "0000-0001-0000-0000"} + ]) + assert creators == [ + {"name": "Ada Lovelace", "orcid": "0000-0001-0000-0000"} + ] + + +def test_repairs_a_plural_affiliation_key(): + creators = zenodo_creators([ + {"name": "Ada Lovelace", "affiliations": "Royal Society"} + ]) + assert creators == [ + {"name": "Ada Lovelace", "affiliation": "Royal Society"} + ] + + +def test_an_exact_field_wins_over_a_repaired_one(): + # Deterministic regardless of dict order: the exact key is authoritative. + creators = zenodo_creators([ + {"name": "Ada Lovelace", "affiliations": "Wrong", "affiliation": "Right"} + ]) + assert creators == [{"name": "Ada Lovelace", "affiliation": "Right"}] + + creators = zenodo_creators([ + {"name": "Ada Lovelace", "affiliation": "Right", "affiliations": "Wrong"} + ]) + assert creators == [{"name": "Ada Lovelace", "affiliation": "Right"}] + + +def test_drops_blank_values(): + creators = zenodo_creators([ + {"name": "Ada Lovelace", "orcid": None, "affiliation": ""} + ]) + assert creators == [{"name": "Ada Lovelace"}] + + +def test_a_bare_string_author_becomes_a_named_creator(): + assert zenodo_creators(["Ada Lovelace"]) == [{"name": "Ada Lovelace"}] + + +def test_stringifies_a_non_string_scalar(): + creators = zenodo_creators([{"name": "Ada Lovelace", "affiliation": 1}]) + assert creators == [{"name": "Ada Lovelace", "affiliation": "1"}] + + +def test_skips_an_author_with_no_name(): + # Zenodo requires a name on every creator. Sending one without would fail + # the whole deposit; dropping it loses one creator instead of all of them. + creators = zenodo_creators([ + {"orcid": "0000-0001-0000-0000"}, + {"name": "Ada Lovelace"}, + ]) + assert creators == [{"name": "Ada Lovelace"}] + + +def test_warns_about_an_author_with_no_name(caplog): + with caplog.at_level("WARNING"): + zenodo_creators([{"orcid": "0000-0001-0000-0000"}]) + assert "no name" in caplog.text + + +def test_tolerates_junk_input(): + assert zenodo_creators(None) == [] + assert zenodo_creators([]) == [] + assert zenodo_creators("not a list") == [] + + +def test_does_not_mutate_the_caller_s_authors(): + authors = [{"name": "Ada Lovelace", "email": "ada@example.org"}] + zenodo_creators(authors) + assert authors == [{"name": "Ada Lovelace", "email": "ada@example.org"}] + + +def test_result_is_json_serialisable(): + creators = zenodo_creators([ + {"name": "Ada Lovelace", "orcid": "0000-0001-0000-0000"} + ]) + assert json.loads(json.dumps(creators)) == creators + + +def test_allowed_fields_are_the_zenodo_legacy_creator_schema(): + assert ZENODO_CREATOR_FIELDS == ("name", "affiliation", "orcid", "gnd") From 49b3ef26907267af47b288bf22cc302f97f83d37 Mon Sep 17 00:00:00 2001 From: Agah Date: Wed, 19 Aug 2026 19:15:26 -0400 Subject: [PATCH 10/11] Look the docker image up under its registry namespace zenodo_upload_docker_task could not find an image that was sitting in the registry. It asked for binder-roboneurolibre-2doct-2dt1-2dpaper-f65dae; the repository is registry.evidencepub.io/binder-roboneurolibre-2doct-2dt1 -2dpaper-f65dae. The registry host doubles as the repository namespace -- the "registry url entered twice" the docker_save call below already notes -- and bh_project_name is the REES config that prepends it. This task was the only REES call site that omitted it; preview_build_myst_task passes it and resolves its images fine. The name was otherwise correct: f65dae is sha256("roboneurolibre-oct-t1-paper")[:6] and the -2d encoding matches, so the lookup 404'd on a path that was one segment short rather than on an absent image. The catalog fallback missed it for the same reason, its pattern being built from the same config. found_image_name now carries the namespace, which is what the image_name below expects: prepending the host to it yields host/namespace/repo:tag, the same reference myst-libre's pull_image builds. Also report the failure to GitHub. Since myst-libre 0.4.1 a missing image raises ImageNotFoundError from REES.__init__ rather than returning False from the search, so the one branch that called task.fail became unreachable and the error escaped as a bare traceback -- Celery recorded the failure and the issue comment sat orange indefinitely, saying nothing. That is why this looked like a hang rather than a 404. Claude-Session: https://claude.ai/code/session_016Tcpjta8yB3my2RLeH5tty --- api/neurolibre_celery_tasks.py | 47 ++++++++++++++++++++++++---------- 1 file changed, 33 insertions(+), 14 deletions(-) diff --git a/api/neurolibre_celery_tasks.py b/api/neurolibre_celery_tasks.py index 9eeaef3..6b911bd 100644 --- a/api/neurolibre_celery_tasks.py +++ b/api/neurolibre_celery_tasks.py @@ -23,6 +23,7 @@ from repo2data.repo2data import Repo2Data from myst_libre.tools import JupyterHubLocalSpawner from myst_libre.rees import REES +from myst_libre.exceptions import MystLibreError from myst_libre.builders import MystBuilder from myst_libre.tools import MystMD from celery.schedules import crontab @@ -1262,20 +1263,38 @@ def zenodo_upload_docker_task(self, screening_dict): task.fail(f"ERROR: Unrecognized archive type.") else: - # try: - rees_resources = REES(dict( - registry_url=BINDER_REGISTRY, - gh_user_repo_name = f"{GH_ORGANIZATION}/{task.repo_name}", - gh_repo_commit_hash = commit_fork, - binder_image_tag = commit_fork, - binder_image_name = None, - dotenv = task.get_dotenv_path())) - - if rees_resources.search_img_by_repo_name(): - logging.info(f"🐳 FOUND IMAGE... ⬇️ PULLING {rees_resources.found_image_name}") - rees_resources.pull_image() - else: - task.fail(f"Failes REES docker image pull for {fork_url}") + # REES discovers the image in its constructor, and since myst-libre + # 0.4.1 a missing one raises ImageNotFoundError instead of reporting + # False. Unhandled, that escaped as a bare traceback: Celery marked the + # task failed but nothing told GitHub, so the issue comment sat orange + # forever with no indication anything had gone wrong. + try: + rees_resources = REES(dict( + registry_url=BINDER_REGISTRY, + gh_user_repo_name = f"{GH_ORGANIZATION}/{task.repo_name}", + # The registry host doubles as the repository namespace -- the + # "registry url entered twice" noted below -- so the image lives + # at registry.evidencepub.io/binder--, not at + # binder--. bh_project_name is what prepends it. + # Without it the tags/list lookup 404s on an image that exists, + # which preview_build_myst_task gets right and this did not. + bh_project_name = BINDER_REGISTRY.split('https://')[-1], + gh_repo_commit_hash = commit_fork, + binder_image_tag = commit_fork, + binder_image_name = None, + dotenv = task.get_dotenv_path())) + + if rees_resources.search_img_by_repo_name(): + logging.info(f"🐳 FOUND IMAGE... ⬇️ PULLING {rees_resources.found_image_name}") + rees_resources.pull_image() + else: + # Retained for a myst-libre that still reports absence by + # returning False rather than raising. + task.fail(f"Failed REES docker image pull for {fork_url}") + return + except MystLibreError as exception: + task.fail(f"Cannot pull the docker image for {fork_url} from {BINDER_REGISTRY}: {exception}") + return # except: From cd6393fd63f0d5206a41e2efef7fed1de1f7582e Mon Sep 17 00:00:00 2001 From: Agah Date: Wed, 19 Aug 2026 19:26:22 -0400 Subject: [PATCH 11/11] Drop the second image lookup from the docker upload With the namespace fixed the image resolved, and the next line raised AttributeError: 'REES' object has no attribute 'search_img_by_repo_name'. That method is on DockerRegistryClient. It was reachable through REES in an earlier myst-libre; in 0.4.1 discovery moved into REES.__init__, so the call is both wrong and redundant -- reaching it means the constructor already found the image, because it raises when it does not. Also catch anything else the block can raise. An AttributeError is not a MystLibreError, so the guard added with the namespace fix did not cover it, and the issue comment went orange and silent again -- the same failure mode one exception class over. This task's only channel to the submitter is that comment, so report the exception class there rather than leaving it in a worker log nobody is watching. Claude-Session: https://claude.ai/code/session_016Tcpjta8yB3my2RLeH5tty --- api/neurolibre_celery_tasks.py | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/api/neurolibre_celery_tasks.py b/api/neurolibre_celery_tasks.py index 6b911bd..e23d60d 100644 --- a/api/neurolibre_celery_tasks.py +++ b/api/neurolibre_celery_tasks.py @@ -1284,17 +1284,23 @@ def zenodo_upload_docker_task(self, screening_dict): binder_image_name = None, dotenv = task.get_dotenv_path())) - if rees_resources.search_img_by_repo_name(): - logging.info(f"🐳 FOUND IMAGE... ⬇️ PULLING {rees_resources.found_image_name}") - rees_resources.pull_image() - else: - # Retained for a myst-libre that still reports absence by - # returning False rather than raising. - task.fail(f"Failed REES docker image pull for {fork_url}") - return + # No second lookup: the constructor above already discovered the + # image and raises when it is absent, so reaching here means it was + # found. search_img_by_repo_name lives on the registry client, not + # on REES, and calling it here raised AttributeError. + logging.info(f"🐳 FOUND IMAGE... ⬇️ PULLING {rees_resources.found_image_name}") + rees_resources.pull_image() except MystLibreError as exception: task.fail(f"Cannot pull the docker image for {fork_url} from {BINDER_REGISTRY}: {exception}") return + except Exception as exception: + # This task's only channel to the submitter is the issue comment. + # Anything unhandled here used to leave it orange forever while + # Celery logged a traceback nobody was watching, so report the + # class of the error too rather than letting it escape. + task.fail(f"Unexpected error preparing the docker image for {fork_url}: " + f"{exception.__class__.__name__}: {exception}") + return # except: