From f0d9c6ee52016fbbc320009d764a834e62c2a5e0 Mon Sep 17 00:00:00 2001 From: Seyed Yahya Shirazi Date: Thu, 14 May 2026 11:29:48 -0700 Subject: [PATCH 01/39] Bump version to 0.8.4.dev0 Manual recovery: sync-develop.yml could not run because develop was auto-deleted after the v0.8.3 release PR merged (repo had delete_branch_on_merge=true). Reproducing what that workflow would have done: develop = main + patch dev0 bump. --- src/version.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/version.py b/src/version.py index d79d2d1..f689bfc 100644 --- a/src/version.py +++ b/src/version.py @@ -1,7 +1,7 @@ """Version information for OSA.""" -__version__ = "0.8.3" -__version_info__ = (0, 8, 3) +__version__ = "0.8.4.dev0" +__version_info__ = (0, 8, 4, "dev") def get_version() -> str: From 0d50dc50cc99ed06fd4606db3f4724c16ec02e30 Mon Sep 17 00:00:00 2001 From: "Seyed (Yahya) Shirazi" Date: Sat, 16 May 2026 10:24:19 -0700 Subject: [PATCH 02/39] feat: document versioned widget URL and SRI hashes for secure embedding (#279) * feat: document versioned widget URL and SRI hashes for secure embedding Add security-sensitive embedding section to the widget demo page showing the versioned jsDelivr URL and SRI hash approach as an alternative to the default (always-latest) URL. The default remains the recommended approach; the versioned option is clearly labeled for supply-chain-secure workflows. Also add scripts/widget-sri.py to generate SHA-384 SRI hashes for any release, useful for maintainers publishing release notes. Closes #268 Co-authored-by: Seyed (Yahya) Shirazi * Enhance release notes with widget SRI hash Append widget SRI hash to release notes and update embedding instructions. --------- Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: Seyed (Yahya) Shirazi --- .github/workflows/release.yml | 32 ++++++++++++++++++ frontend/index.html | 22 +++++++++++++ scripts/widget-sri.py | 61 +++++++++++++++++++++++++++++++++++ 3 files changed, 115 insertions(+) create mode 100644 scripts/widget-sri.py diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index a6332f7..758c515 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -58,6 +58,38 @@ jobs: EOF echo "Generated release notes for v${{ steps.version.outputs.version }}" + - name: Append widget SRI hash to release notes + run: | + VERSION="${{ steps.version.outputs.version }}" + HASH=$(python3 -c " + import hashlib, base64 + with open('frontend/osa-chat-widget.js', 'rb') as f: + content = f.read() + print('sha384-' + base64.b64encode(hashlib.sha384(content).digest()).decode()) + ") + python3 - < + \`\`\` + + **Versioned + SRI** (for supply-chain-secure environments): + \`\`\`html + + \`\`\` + > Pin to this exact release. Update the tag when upgrading. + """ + pathlib.Path('/tmp/release_notes.md').write_text(notes + section) + PYEOF - name: Create GitHub Release uses: softprops/action-gh-release@v2 diff --git a/frontend/index.html b/frontend/index.html index 3f76f44..da81881 100644 --- a/frontend/index.html +++ b/frontend/index.html @@ -391,6 +391,17 @@

Integration

}); </script>

The widget auto-configures the API endpoint based on the communityId.

+
+

Security-sensitive environments (supply-chain policies, SRI required): pin to a specific release using a versioned jsDelivr URL. The integrity hash for each release is published in the GitHub releases.

+ 
<!-- Replace vX.Y.Z and the integrity hash from the GitHub release notes -->
+<script src="https://cdn.jsdelivr.net/gh/OpenScience-Collective/osa@vX.Y.Z/frontend/osa-chat-widget.js"
+        integrity="sha384-..."
+        crossorigin="anonymous"
+        defer></script>
+<script>
+  OSAChatWidget.setConfig({ communityId: 'hed' });
+</script>
+

Note: you must update the version tag manually when upgrading.

@@ -479,6 +490,17 @@

Add to Your Site

// widgetInstructions: 'Focus on topics relevant to this page.' }); </script> +
+

Security-sensitive environments (supply-chain policies, SRI required): use a versioned URL pinned to a specific release. The integrity hash for each release is published in the GitHub releases.

+ 
<!-- Replace vX.Y.Z and the integrity hash from the GitHub release notes -->
+<script src="https://cdn.jsdelivr.net/gh/OpenScience-Collective/osa@vX.Y.Z/frontend/osa-chat-widget.js"
+        integrity="sha384-..."
+        crossorigin="anonymous"
+        defer></script>
+<script>
+  OSAChatWidget.setConfig({ communityId: '${communityId}' });
+</script>
+

Note: you must update the version tag manually when upgrading.

diff --git a/scripts/widget-sri.py b/scripts/widget-sri.py new file mode 100644 index 0000000..29b913c --- /dev/null +++ b/scripts/widget-sri.py @@ -0,0 +1,61 @@ +#!/usr/bin/env python3 +"""Generate the SRI hash for the OSA chat widget. + +Usage: + # Hash the local file (useful during development / CI before a release is tagged) + python scripts/widget-sri.py + + # Hash a specific release from jsDelivr (useful after tagging) + python scripts/widget-sri.py v0.8.3 + +The script prints the sha384 integrity value and a ready-to-paste embed snippet. +""" + +import base64 +import hashlib +import sys +import urllib.request +from pathlib import Path + + +def sri(content: bytes) -> str: + return "sha384-" + base64.b64encode(hashlib.sha384(content).digest()).decode() + + +def main() -> None: + if len(sys.argv) > 1: + tag = sys.argv[1] if sys.argv[1].startswith("v") else f"v{sys.argv[1]}" + url = ( + f"https://cdn.jsdelivr.net/gh/OpenScience-Collective/osa@{tag}" + "/frontend/osa-chat-widget.js" + ) + print(f"Fetching {url} …", flush=True) + with urllib.request.urlopen(url) as resp: # noqa: S310 (trusted CDN URL) + content = resp.read() + label = f"jsDelivr {tag}" + else: + widget_path = Path(__file__).parent.parent / "frontend" / "osa-chat-widget.js" + if not widget_path.exists(): + print(f"Error: {widget_path} not found.", file=sys.stderr) + print("Pass a version tag to fetch from jsDelivr instead:", file=sys.stderr) + print(" python scripts/widget-sri.py v0.8.3", file=sys.stderr) + sys.exit(1) + content = widget_path.read_bytes() + tag = None + label = f"local ({widget_path.name})" + + hash_value = sri(content) + print(f"\nSRI hash ({label}):") + print(f" integrity=\"{hash_value}\"") + + if tag: + print(f"\nVersioned embed snippet for {tag}:") + print(f"""\ + """) + + +if __name__ == "__main__": + main() From 7fc024ea12ba047b6ff1bcc0af70f2a6ce80fdd7 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Sat, 16 May 2026 17:24:30 +0000 Subject: [PATCH 03/39] Bump version to 0.8.4.dev1 --- src/version.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/version.py b/src/version.py index f689bfc..77e2e53 100644 --- a/src/version.py +++ b/src/version.py @@ -1,7 +1,7 @@ """Version information for OSA.""" -__version__ = "0.8.4.dev0" -__version_info__ = (0, 8, 4, "dev") +__version__ = "0.8.4.dev1" +__version_info__ = (0, 8, 4, "dev1") def get_version() -> str: From f19ed085ce0c9d85f8ec5f4d90c279604653aa3c Mon Sep 17 00:00:00 2001 From: Granville Matheson Date: Tue, 2 Jun 2026 22:16:01 +0200 Subject: [PATCH 04/39] feat: add petfit readthedocs documentation to openneuropet assistant (#281) --- src/assistants/openneuropet/config.yaml | 73 +++++++++++++++++++++++-- 1 file changed, 69 insertions(+), 4 deletions(-) diff --git a/src/assistants/openneuropet/config.yaml b/src/assistants/openneuropet/config.yaml index 6cc93e9..fa22a38 100644 --- a/src/assistants/openneuropet/config.yaml +++ b/src/assistants/openneuropet/config.yaml @@ -133,11 +133,76 @@ documentation: source_url: https://raw.githubusercontent.com/nipreps/petprep/main/docs/faq.rst category: analysis description: Frequently asked questions about PETPrep usage, data quality requirements, and preprocessing decisions. - - title: petfit documentation - url: https://github.com/mathesong/petfit - source_url: https://raw.githubusercontent.com/mathesong/petfit/main/README.md + - title: petfit overview + url: https://petfit.readthedocs.io/en/latest/ + source_url: https://raw.githubusercontent.com/mathesong/petfit/main/docs/index.md category: analysis - description: Petfit for regional kinetic modelling (based on kinfitr). + description: PETFit is a BIDS App for fitting kinetic models to PET time activity curve (TAC) data, producing parameter estimates and QC reports. Supports interactive (GUI) and automatic (CLI) modes. + - title: petfit installation + url: https://petfit.readthedocs.io/en/latest/installation.html + source_url: https://raw.githubusercontent.com/mathesong/petfit/main/docs/installation.md + category: analysis + description: Installing PETFit via Docker (recommended), Apptainer, or the R package. + - title: petfit quick start + url: https://petfit.readthedocs.io/en/latest/quickstart.html + source_url: https://raw.githubusercontent.com/mathesong/petfit/main/docs/quickstart.md + category: analysis + description: Two-step PETFit workflow - region definition (once per dataset) then kinetic modelling - run interactively or from the command line. + - title: petfit usage guide + url: https://petfit.readthedocs.io/en/latest/usage/index.html + source_url: https://raw.githubusercontent.com/mathesong/petfit/main/docs/usage/index.md + category: analysis + description: Overview of the region-definition and kinetic-modelling stages and the three Shiny apps (region definition, plasma input, reference tissue). + - title: petfit region definition + url: https://petfit.readthedocs.io/en/latest/usage/region-definition.html + source_url: https://raw.githubusercontent.com/mathesong/petfit/main/docs/usage/region-definition.md + category: analysis + description: First step of a PETFit workflow - combining individual brain regions from PET preprocessing derivatives into analysis-ready TACs via the petfit_regions.tsv file. + - title: petfit plasma input modelling + url: https://petfit.readthedocs.io/en/latest/usage/modelling-plasma.html + source_url: https://raw.githubusercontent.com/mathesong/petfit/main/docs/usage/modelling-plasma.md + category: analysis + description: Configuring and running invasive kinetic models that require an arterial blood input function (raw _blood.tsv or processed _inputfunction.tsv data). + - title: petfit reference tissue modelling + url: https://petfit.readthedocs.io/en/latest/usage/modelling-reference.html + source_url: https://raw.githubusercontent.com/mathesong/petfit/main/docs/usage/modelling-reference.md + category: analysis + description: Configuring and running non-invasive kinetic models that use a reference brain region instead of arterial blood data. + - title: petfit supported models + url: https://petfit.readthedocs.io/en/latest/models.html + source_url: https://raw.githubusercontent.com/mathesong/petfit/main/docs/models.md + category: analysis + description: Kinetic models available in PETFit (via kinfitr) - plasma input models (1TCM, 2TCM, Logan, etc.) and reference tissue models. + - title: petfit reports + url: https://petfit.readthedocs.io/en/latest/usage/reports.html + source_url: https://raw.githubusercontent.com/mathesong/petfit/main/docs/usage/reports.md + category: analysis + description: Parameterised HTML quality-control reports generated for every analysis step, with interactive plots, data summaries, and diagnostics. + - title: petfit outputs + url: https://petfit.readthedocs.io/en/latest/outputs.html + source_url: https://raw.githubusercontent.com/mathesong/petfit/main/docs/outputs.md + category: analysis + description: PETFit output files following BIDS derivatives conventions in derivatives/petfit/, including parameter estimates and HTML reports. + - title: petfit folder structure + url: https://petfit.readthedocs.io/en/latest/usage/folder-structure.html + source_url: https://raw.githubusercontent.com/mathesong/petfit/main/docs/usage/folder-structure.md + category: analysis + description: How PETFit organises its outputs in a structured hierarchy within the BIDS derivatives directory. + - title: petfit Docker usage + url: https://petfit.readthedocs.io/en/latest/containers/docker.html + source_url: https://raw.githubusercontent.com/mathesong/petfit/main/docs/containers/docker.md + category: analysis + description: Advanced Docker options and CLI reference for running PETFit container images. + - title: petfit Apptainer usage + url: https://petfit.readthedocs.io/en/latest/containers/apptainer.html + source_url: https://raw.githubusercontent.com/mathesong/petfit/main/docs/containers/apptainer.md + category: analysis + description: Running PETFit with Apptainer (formerly Singularity), the standard container runtime on HPC clusters. + - title: petfit R API reference + url: https://petfit.readthedocs.io/en/latest/api.html + source_url: https://raw.githubusercontent.com/mathesong/petfit/main/docs/api.md + category: analysis + description: Reference for PETFit's main R functions for launching apps and running pipelines (e.g. petfit_interactive()). - title: PetSurfer Wiki url: https://surfer.nmr.mgh.harvard.edu/fswiki/PetSurfer source_url: https://surfer.nmr.mgh.harvard.edu/fswiki/PetSurfer From c2967a6c3e5255ed5f264095dfb14f14585e4447 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Tue, 2 Jun 2026 20:16:17 +0000 Subject: [PATCH 05/39] Bump version to 0.8.4.dev2 --- src/version.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/version.py b/src/version.py index 77e2e53..d7db3dc 100644 --- a/src/version.py +++ b/src/version.py @@ -1,7 +1,7 @@ """Version information for OSA.""" -__version__ = "0.8.4.dev1" -__version_info__ = (0, 8, 4, "dev1") +__version__ = "0.8.4.dev2" +__version_info__ = (0, 8, 4, "dev2") def get_version() -> str: From 8d13bf520a33427be076918521692872ecd9c8d1 Mon Sep 17 00:00:00 2001 From: "Seyed (Yahya) Shirazi" Date: Thu, 4 Jun 2026 19:35:30 -0700 Subject: [PATCH 06/39] feat: per-response and general widget feedback (#284) * feat: per-response and general widget feedback Widget: thumbs up/down under each assistant reply (sentiment only, one-vote-lock, keyed to a new per-response request_id) plus a Send-feedback footer link opening a free-text modal for general feedback. Backend: anonymous POST /feedback (worker already proxies to it) writing to a new feedback_log table in the existing metrics.db (auto-created via CREATE TABLE IF NOT EXISTS; rides the osa-data volume). request_id is now exposed on the chat/ask done event and ChatResponse/AskResponse. page_url is scheme-validated to prevent stored XSS in the admin view. Admin: GET /metrics/feedback (per-community scoped key, strictly confined) and a Feedback panel on the status dashboard. Tested: 23 new tests (db/queries + endpoint scoping); ruff clean. Closes #282 * refactor: address PR #284 review findings - FeedbackEntry: narrow to Literal types + __post_init__ enforcing the per-type invariants at the storage layer (last checkpoint before SQLite); add CHECK constraints + a UNIQUE index on feedback_id to the schema. - feedback.py: move sentiment/comment normalization into a mode=before validator (drop object.__setattr__); remove the unreachable manual whitelist checks and the dead try/except (write_feedback never raises). - write_feedback: acquire the connection inside the try so it honors its never-raises contract on a connect failure. - Widget: submitResponseFeedback now awaits the POST and rolls back the optimistic vote (with an error) if it fails, instead of silently dropping. - Tests: replace the mock-based fixture with a DATA_DIR env fixture; add __post_init__ invariant tests, write-failure counter + CRITICAL escalation, comments_only/offset/limit-floor coverage, data:/ftp page_url rejection, general-sentiment-stripping, request_id round-trip, cross-community admin aggregation; fix the no-op test_limit_clamped. - Docs/comments: db module docstring, warning SSE event, request_id wording, widget/dashboard comment and error-message fixes. --- dashboard/osa/index.html | 202 ++++++++++++++ frontend/osa-chat-widget.js | 300 +++++++++++++++++++- src/api/main.py | 6 + src/api/routers/__init__.py | 2 + src/api/routers/community.py | 38 ++- src/api/routers/feedback.py | 145 ++++++++++ src/api/routers/metrics.py | 43 +++ src/metrics/db.py | 137 ++++++++- src/metrics/queries.py | 116 ++++++++ tests/test_api/test_feedback_endpoint.py | 339 +++++++++++++++++++++++ tests/test_metrics/test_feedback_db.py | 320 +++++++++++++++++++++ 11 files changed, 1638 insertions(+), 10 deletions(-) create mode 100644 src/api/routers/feedback.py create mode 100644 tests/test_api/test_feedback_endpoint.py create mode 100644 tests/test_metrics/test_feedback_db.py diff --git a/dashboard/osa/index.html b/dashboard/osa/index.html index 122882c..991725e 100644 --- a/dashboard/osa/index.html +++ b/dashboard/osa/index.html @@ -416,6 +416,50 @@ .admin-status.success { color: #059669; } .admin-status.error { color: #dc2626; } + /* Feedback panel */ + .feedback-toolbar { + display: flex; + align-items: center; + gap: 0.5rem; + margin-bottom: 0.75rem; + font-size: 0.82rem; + color: #64748b; + } + .feedback-toolbar label { display: inline-flex; align-items: center; gap: 0.4rem; cursor: pointer; } + .feedback-table-wrap { + max-height: 320px; + overflow-y: auto; + border: 1px solid #e2e8f0; + border-radius: 8px; + } + .feedback-table { + width: 100%; + border-collapse: collapse; + font-size: 0.82rem; + } + .feedback-table th, .feedback-table td { + text-align: left; + padding: 0.5rem 0.7rem; + border-bottom: 1px solid #e2e8f0; + vertical-align: top; + } + .feedback-table th { + position: sticky; + top: 0; + background: #f8fafc; + color: #475569; + font-weight: 600; + font-size: 0.72rem; + text-transform: uppercase; + letter-spacing: 0.03em; + } + .feedback-table tr:last-child td { border-bottom: none; } + .feedback-table td a { color: #2563eb; text-decoration: none; } + .feedback-table td a:hover { text-decoration: underline; } + .feedback-sentiment-up { color: #059669; font-weight: 600; } + .feedback-sentiment-down { color: #dc2626; font-weight: 600; } + .feedback-empty { color: #64748b; padding: 1rem; font-size: 0.85rem; } + /* Loading / error */ .loading { text-align: center; color: #64748b; padding: 2rem; } .loading::after { @@ -508,6 +552,14 @@ .admin-btn { background: #7ba3d4; color: #0d1117; } .admin-btn:hover { background: #93b8de; } .loading { color: #8a9bb5; } + .feedback-toolbar { color: #8a9bb5; } + .feedback-table-wrap { border-color: #30363d; } + .feedback-table th, .feedback-table td { border-color: #30363d; } + .feedback-table th { background: #1c2128; color: #8a9bb5; } + .feedback-table td a { color: #7ba3d4; } + .feedback-sentiment-up { color: #4ade80; } + .feedback-sentiment-down { color: #f87171; } + .feedback-empty { color: #8a9bb5; } .error-msg { color: #f87171; background: #3b1219; border-color: #5c1d2a; } .site-footer { color: #6b7b92; border-color: #30363d; } .site-footer a { color: #7ba3d4; } @@ -918,6 +970,8 @@

+

Admin: Feedback

+
Loading feedback...
`; @@ -1167,6 +1221,8 @@

Failed to load feedback (HTTP ${resp.status})

`; + } + return; + } + renderAdminFeedback(await resp.json(), communityId); + } catch (err) { + console.error('Failed to load feedback:', err); + container.innerHTML = '

Failed to load feedback: unexpected server response. Try refreshing the page.

'; + } + } + + function toggleFeedbackCommentsOnly(checked, communityId) { + feedbackCommentsOnly = checked; + loadAdminFeedback(decodeURIComponent(communityId)); + } + + function formatFeedbackTime(isoStr) { + if (!isoStr) return '—'; + try { + const d = new Date(isoStr); + if (isNaN(d.getTime())) return isoStr; + return d.toLocaleString(undefined, { + year: 'numeric', month: 'short', day: 'numeric', + hour: '2-digit', minute: '2-digit', + }); + } catch (err) { console.warn('Failed to parse feedback timestamp:', isoStr, err); return isoStr; } + } + + function renderAdminFeedback(data, communityId) { + const container = document.getElementById('adminFeedback'); + if (!container) return; + + const summary = data.summary || {}; + const up = summary.thumbs_up || 0; + const down = summary.thumbs_down || 0; + const satRate = (summary.satisfaction_rate === null || summary.satisfaction_rate === undefined) + ? '—' : `${(summary.satisfaction_rate * 100).toFixed(0)}%`; + const responseTotal = summary.response_total || 0; + const generalTotal = summary.general_total || 0; + const commentTotal = summary.comment_total || 0; + + const summaryHtml = ` +
+
+ +
Thumbs Up
+
+
+ +
Thumbs Down
+
+
+
${satRate}
+
Satisfaction Rate
+
+
+
${responseTotal.toLocaleString()}
+
Responses Rated
+
+
+
${generalTotal.toLocaleString()}
+
General Feedback
+
+
+
${commentTotal.toLocaleString()}
+
Comments
+
+
`; + + const safeCommunity = encodeURIComponent(communityId); + const toolbarHtml = ` + `; + + const entries = Array.isArray(data.entries) ? data.entries : []; + let tableHtml; + if (entries.length === 0) { + tableHtml = ''; + } else { + const rows = entries.map(e => { + const time = escapeHtml(formatFeedbackTime(e.timestamp)); + const type = escapeHtml(e.feedback_type || '—'); + let sentiment = ''; + if (e.sentiment === 'up') sentiment = ''; + else if (e.sentiment === 'down') sentiment = ''; + const comment = e.comment ? escapeHtml(e.comment) : '—'; + // Defense in depth: only render a clickable link for http(s) URLs + // (the backend already enforces this on write). + const isHttpUrl = typeof e.page_url === 'string' && /^https?:\/\//i.test(e.page_url); + const page = isHttpUrl + ? `link` + : ''; + return ` + ${time} + ${type} + ${sentiment} + ${comment} + ${page} + `; + }).join(''); + tableHtml = ` + `; + } + + container.innerHTML = summaryHtml + toolbarHtml + tableHtml; + } + function renderAdminCharts(data) { if (adminTokenChartInstance) adminTokenChartInstance.destroy(); const tokenCanvas = document.getElementById('adminTokenChart'); diff --git a/frontend/osa-chat-widget.js b/frontend/osa-chat-widget.js index 1df9f59..0f8ae9a 100644 --- a/frontend/osa-chat-widget.js +++ b/frontend/osa-chat-widget.js @@ -118,7 +118,9 @@ copy: '', check: '', popout: '', - settings: '' + settings: '', + thumbUp: '', + thumbDown: '' }; // CSS Styles @@ -569,6 +571,64 @@ gap: 8px; } + .osa-message-feedback { + display: flex; + align-items: center; + gap: 4px; + margin-top: 6px; + } + + .osa-feedback-btn { + display: inline-flex; + align-items: center; + justify-content: center; + width: 26px; + height: 26px; + padding: 0; + border: none; + border-radius: 6px; + background: transparent; + color: var(--osa-text-light); + cursor: pointer; + transition: background 0.15s ease, color 0.15s ease; + } + + .osa-feedback-btn svg { + width: 15px; + height: 15px; + } + + .osa-feedback-btn:hover { + background: var(--osa-assistant-bg); + color: var(--osa-text); + } + + .osa-feedback-up.selected { + color: #16a34a; + } + + .osa-feedback-down.selected { + color: #dc2626; + } + + .osa-message-feedback.recorded .osa-feedback-btn { + cursor: default; + } + + .osa-message-feedback.recorded .osa-feedback-btn:not(.selected) { + opacity: 0.3; + } + + .osa-message-feedback.recorded .osa-feedback-btn:hover { + background: transparent; + } + + .osa-feedback-thanks { + font-size: 12px; + color: var(--osa-text-light); + margin-left: 4px; + } + .osa-suggestions { padding: 12px 16px; border-top: 1px solid var(--osa-border); @@ -810,6 +870,38 @@ text-decoration: underline; } + .osa-combined-footer .osa-footer-powered .osa-feedback-link { + background: none; + border: none; + padding: 0; + font: inherit; + color: var(--osa-text-light); + cursor: pointer; + text-decoration: none; + } + + .osa-combined-footer .osa-footer-powered .osa-feedback-link:hover { + color: var(--osa-primary); + text-decoration: underline; + } + + .osa-combined-footer .osa-footer-powered .osa-footer-sep { + margin: 0 4px; + color: var(--osa-border); + } + + .osa-feedback-textarea { + resize: vertical; + min-height: 80px; + font-family: inherit; + } + + .osa-feedback-modal-thanks { + padding: 16px 20px; + color: #16a34a; + font-weight: 600; + } + /* Settings modal - contained within chat window to avoid z-index conflicts and ensure modal is properly scoped to the widget's stacking context */ .osa-settings-overlay { @@ -1766,6 +1858,117 @@ closeSettings(container); } + // --- Feedback ----------------------------------------------------------- + + // Low-level POST to the feedback endpoint. In production the request goes + // through the Cloudflare Worker proxy; in development CONFIG.apiEndpoint + // points directly to the backend. Best-effort: never throws to the caller. + async function postFeedback(payload) { + try { + const response = await fetch(`${CONFIG.apiEndpoint}/feedback`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ community_id: CONFIG.communityId, ...payload }), + }); + if (!response.ok) { + console.warn('[OSA] Feedback submission returned', response.status); + return false; + } + return true; + } catch (e) { + console.error('[OSA] Failed to submit feedback:', e); + return false; + } + } + + // Record a thumbs up/down on a specific assistant reply. One vote per reply + // per browser session (stored in localStorage): once recorded, the choice is + // locked in this browser to keep counts honest. The button updates optimistically + // for responsiveness, but if the POST fails the vote is rolled back so the user + // is not misled into thinking it was saved. + async function submitResponseFeedback(container, msgIndex, sentiment) { + const msg = messages[msgIndex]; + if (!msg || msg.role !== 'assistant') return; + if (msg.feedback) return; // already voted on this reply + if (sentiment !== 'up' && sentiment !== 'down') return; + + // Optimistic update. + msg.feedback = sentiment; + renderMessages(container); + + const ok = await postFeedback({ + feedback_type: 'response', + sentiment, + request_id: msg.requestId || null, + session_id: sessionId || null, + message_index: msgIndex, + }); + + if (!ok) { + // Roll back so the vote can be retried and the user knows it did not save. + delete msg.feedback; + renderMessages(container); + showError(container, 'Could not send feedback. Please try again.'); + return; + } + + try { + saveHistory(); + } catch (e) { + console.error('[OSA] Failed to persist feedback locally:', e); + } + } + + // Open the general (free-text) feedback modal + function openFeedback(container) { + const overlay = container.querySelector('.osa-feedback-overlay'); + if (!overlay) return; + const textarea = container.querySelector('#osa-feedback-text'); + const thanks = container.querySelector('.osa-feedback-modal-thanks'); + const form = container.querySelector('.osa-feedback-modal-form'); + if (textarea) textarea.value = ''; + if (thanks) thanks.style.display = 'none'; + if (form) form.style.display = 'block'; + overlay.classList.add('open'); + if (textarea) textarea.focus(); + } + + function closeFeedback(container) { + const overlay = container.querySelector('.osa-feedback-overlay'); + if (overlay) overlay.classList.remove('open'); + } + + // Send free-text general feedback (not tied to a single reply) + async function submitGeneralFeedback(container) { + const textarea = container.querySelector('#osa-feedback-text'); + const comment = textarea ? textarea.value.trim() : ''; + if (!comment) { + showError(container, 'Please enter some feedback first.'); + return; + } + if (comment.length > 5000) { + showError(container, 'Feedback is too long (5000 character max).'); + return; + } + + const ok = await postFeedback({ + feedback_type: 'general', + comment, + session_id: sessionId || null, + page_url: (typeof window !== 'undefined' && window.location) ? window.location.href : null, + }); + + if (ok) { + const thanks = container.querySelector('.osa-feedback-modal-thanks'); + const form = container.querySelector('.osa-feedback-modal-form'); + if (form) form.style.display = 'none'; + if (thanks) thanks.style.display = 'block'; + setTimeout(() => closeFeedback(container), 1500); + } else { + showError(container, 'Could not send feedback. Please try again later.'); + } + } + // Check backend health status async function checkBackendStatus() { const statusDot = document.querySelector('.osa-status-dot'); @@ -1965,6 +2168,8 @@
+ + · Powered by OSA
@@ -2027,6 +2232,44 @@ + +
+
+
+

Send feedback

+ +
+
+
+ + + + Shared with the ${escapeHtml(CONFIG.title.replace(' Assistant', ''))} community maintainers. Please do not include personal information. + +
+
+ +
+ + +
+
`; @@ -2056,12 +2299,26 @@ ? `` : ''; + // Per-response feedback (thumbs up/down) for assistant replies, but not + // the canned opening greeting (index 0). Records sentiment only; free-text + // feedback lives in the "Send feedback" link in the footer. + const showFeedback = msg.role === 'assistant' && msgIndex > 0; + const fb = msg.feedback; + const feedbackRow = showFeedback + ? `
+ + + +
` + : ''; + msgEl.innerHTML = `
${escapeHtml(label)} ${copyBtn}
${content}
+ ${feedbackRow} `; messagesEl.appendChild(msgEl); }); @@ -2093,6 +2350,17 @@ }); }); + // Per-response thumbs up/down buttons + messagesEl.querySelectorAll('.osa-message-feedback .osa-feedback-btn[data-feedback]').forEach(btn => { + btn.addEventListener('click', (e) => { + e.stopPropagation(); + const row = btn.closest('.osa-message-feedback'); + const msgIndex = parseInt(row.getAttribute('data-msg-index'), 10); + const sentiment = btn.getAttribute('data-feedback'); + submitResponseFeedback(container, msgIndex, sentiment); + }); + }); + if (isLoading) { const loadingEl = document.createElement('div'); loadingEl.className = 'osa-loading'; @@ -2236,7 +2504,9 @@ // Log tool completion console.log('[OSA] Tool completed:', event.name); } else if (event.event === 'session') { - // Capture session ID early (sent at stream start) + // Capture session ID early (sent at stream start). request_id is + // intentionally NOT sent here; it arrives on the 'done' event so it + // only attaches to a reply that completed successfully. if (event.session_id && typeof event.session_id === 'string') { sessionId = event.session_id; } @@ -2251,6 +2521,9 @@ if (event.session_id && typeof event.session_id === 'string') { sessionId = event.session_id; } + if (event.request_id && typeof event.request_id === 'string') { + messages[messageIndex].requestId = event.request_id; + } messages[messageIndex].content = accumulatedContent; renderMessages(container); try { @@ -2473,7 +2746,11 @@ if (!answer) { throw new Error('Invalid response from server'); } - messages.push({ role: 'assistant', content: answer }); + const assistantMsg = { role: 'assistant', content: answer }; + if (data && typeof data.request_id === 'string') { + assistantMsg.requestId = data.request_id; + } + messages.push(assistantMsg); try { saveHistory(); } catch (saveError) { @@ -2852,6 +3129,23 @@ } }); + // Feedback link + modal event listeners + const feedbackLink = container.querySelector('.osa-feedback-link'); + const feedbackOverlay = container.querySelector('.osa-feedback-overlay'); + const feedbackCloseBtn = container.querySelector('.osa-feedback-close-btn'); + const feedbackCancelBtn = container.querySelector('.osa-feedback-cancel-btn'); + const feedbackSendBtn = container.querySelector('.osa-feedback-send-btn'); + + feedbackLink?.addEventListener('click', () => openFeedback(container)); + feedbackCloseBtn?.addEventListener('click', () => closeFeedback(container)); + feedbackCancelBtn?.addEventListener('click', () => closeFeedback(container)); + feedbackSendBtn?.addEventListener('click', () => submitGeneralFeedback(container)); + feedbackOverlay?.addEventListener('click', (e) => { + if (e.target === feedbackOverlay) { + closeFeedback(container); + } + }); + // Show/hide custom model input based on selection modelSelect?.addEventListener('change', (e) => { if (customModelField) { diff --git a/src/api/main.py b/src/api/main.py index 3d486c6..f248c3b 100644 --- a/src/api/main.py +++ b/src/api/main.py @@ -17,6 +17,7 @@ from src.api.routers import ( communities_router, create_community_router, + feedback_router, metrics_public_router, metrics_router, mirrors_router, @@ -272,6 +273,9 @@ def register_routes(app: FastAPI) -> None: # Communities metadata endpoint (public, for widget config) app.include_router(communities_router) + # Feedback endpoint (public; anonymous widget users submit thumbs/comments) + app.include_router(feedback_router) + # Health check router app.include_router(health_router) @@ -323,7 +327,9 @@ async def root() -> dict[str, Any]: endpoints["POST /sync/trigger"] = "Trigger sync (requires API key)" endpoints["GET /metrics/overview"] = "Metrics overview (requires admin key)" endpoints["GET /metrics/tokens"] = "Token breakdown (requires admin key)" + endpoints["GET /metrics/feedback"] = "User feedback (requires admin key)" endpoints["GET /metrics/public/overview"] = "Public metrics overview" + endpoints["POST /feedback"] = "Submit user feedback (thumbs up/down or comment)" endpoints["GET /health"] = "Health check" return { diff --git a/src/api/routers/__init__.py b/src/api/routers/__init__.py index df2c667..d343526 100644 --- a/src/api/routers/__init__.py +++ b/src/api/routers/__init__.py @@ -2,6 +2,7 @@ from src.api.routers.communities import router as communities_router from src.api.routers.community import create_community_router +from src.api.routers.feedback import router as feedback_router from src.api.routers.metrics import router as metrics_router from src.api.routers.metrics_public import router as metrics_public_router from src.api.routers.mirrors import router as mirrors_router @@ -10,6 +11,7 @@ __all__ = [ "communities_router", "create_community_router", + "feedback_router", "metrics_public_router", "metrics_router", "mirrors_router", diff --git a/src/api/routers/community.py b/src/api/routers/community.py index 3e16e5a..0fc6426 100644 --- a/src/api/routers/community.py +++ b/src/api/routers/community.py @@ -145,6 +145,10 @@ class ChatResponse(BaseModel): tool_calls: list[ToolCallInfo] = Field( default_factory=list, description="Tools called during response generation" ) + request_id: str | None = Field( + default=None, + description="Per-request identifier the widget can attach to feedback", + ) class AskResponse(BaseModel): @@ -154,6 +158,10 @@ class AskResponse(BaseModel): tool_calls: list[ToolCallInfo] = Field( default_factory=list, description="Tools called during response generation" ) + request_id: str | None = Field( + default=None, + description="Per-request identifier the widget can attach to feedback", + ) class SessionInfo(BaseModel): @@ -1048,7 +1056,11 @@ async def ask( ar = _extract_agent_result(result) _set_metrics_on_request(http_request, awm, ar) - return AskResponse(answer=ar.response_content, tool_calls=ar.tool_calls_info) + return AskResponse( + answer=ar.response_content, + tool_calls=ar.tool_calls_info, + request_id=getattr(http_request.state, "request_id", None), + ) except HTTPException: raise @@ -1155,6 +1167,7 @@ async def chat( session_id=session.session_id, message=ChatMessage(role="assistant", content=ar.response_content), tool_calls=ar.tool_calls_info, + request_id=getattr(http_request.state, "request_id", None), ) except ValueError as e: @@ -1595,7 +1608,7 @@ async def _stream_ask_response( data: {"event": "content", "content": "text chunk"} data: {"event": "tool_start", "name": "tool_name", "input": {...}} data: {"event": "tool_end", "name": "tool_name", "output": {...}} - data: {"event": "done"} + data: {"event": "done", "request_id": "..."} data: {"event": "error", "message": "error text"} """ start_time = time.monotonic() @@ -1604,6 +1617,9 @@ async def _stream_ask_response( total_input_tokens = 0 total_output_tokens = 0 + # Per-request id (set by metrics middleware) so the widget can attach feedback. + request_id = getattr(http_request.state, "request_id", None) if http_request else None + try: awm = create_community_assistant( community_id, @@ -1658,7 +1674,7 @@ async def _stream_ask_response( } yield f"data: {json.dumps(sse_event)}\n\n" - sse_event = {"event": "done"} + sse_event = {"event": "done", "request_id": request_id} yield f"data: {json.dumps(sse_event)}\n\n" # Log metrics at end of streaming @@ -1767,7 +1783,8 @@ async def _stream_chat_response( data: {"event": "tool_start", "name": "tool_name", "input": {...}} data: {"event": "tool_end", "name": "tool_name", "output": {...}} data: {"event": "session", "session_id": "..."} (sent first) - data: {"event": "done", "session_id": "..."} + data: {"event": "warning", "message": "..."} (optional, before done) + data: {"event": "done", "session_id": "...", "request_id": "..."} data: {"event": "error", "message": "error text"} """ start_time = time.monotonic() @@ -1776,6 +1793,13 @@ async def _stream_chat_response( total_input_tokens = 0 total_output_tokens = 0 + # The metrics middleware assigns a per-request UUID; expose it only on the + # final `done` event (below) so the widget attaches it only to a reply that + # completed normally. Error paths yield an `error` event instead and never + # reach `done`, so a partially-streamed or fully-errored reply carries no + # request_id. This also joins per-response feedback back to request_log. + request_id = getattr(http_request.state, "request_id", None) if http_request else None + # Send session_id immediately so the client captures it even if the # stream is truncated by a proxy timeout. sse_event = {"event": "session", "session_id": session.session_id} @@ -1859,7 +1883,11 @@ async def _stream_chat_response( } yield f"data: {json.dumps(sse_event)}\n\n" - sse_event = {"event": "done", "session_id": session.session_id} + sse_event = { + "event": "done", + "session_id": session.session_id, + "request_id": request_id, + } yield f"data: {json.dumps(sse_event)}\n\n" # Log metrics at end of streaming diff --git a/src/api/routers/feedback.py b/src/api/routers/feedback.py new file mode 100644 index 0000000..392f4e0 --- /dev/null +++ b/src/api/routers/feedback.py @@ -0,0 +1,145 @@ +"""User feedback API endpoint. + +Receives anonymous feedback from the chat widget (per-response thumbs up/down +and free-text general feedback) and persists it to the metrics database for +community admins to review on the status dashboard. + +This is a top-level (non community-prefixed) route because the widget posts to +the Cloudflare Worker's global ``POST /feedback`` proxy, which forwards here; +the community is carried in the request body, not the path. +""" + +import logging +import uuid +from typing import Annotated, Any, Literal + +from fastapi import APIRouter, Header, HTTPException +from pydantic import BaseModel, Field, field_validator, model_validator + +from src.api.routers.community import _is_authorized_origin +from src.assistants import registry +from src.metrics.db import FeedbackEntry, now_iso, write_feedback + +logger = logging.getLogger(__name__) + +router = APIRouter(tags=["Feedback"]) + +_MAX_COMMENT_LEN = 5000 + + +class FeedbackRequest(BaseModel): + """Body for ``POST /feedback`` submitted by the widget. + + Two shapes share this model: + + * ``feedback_type="response"`` -- a thumbs up/down on one reply. ``sentiment`` + is required; ``request_id`` / ``message_index`` link it to the answer. + * ``feedback_type="general"`` -- free-text feedback. ``comment`` is required; + ``sentiment`` is ignored. + """ + + community_id: str = Field(..., min_length=1, max_length=64) + feedback_type: Literal["response", "general"] = "response" + sentiment: Literal["up", "down"] | None = None + request_id: str | None = Field(default=None, max_length=128) + session_id: str | None = Field(default=None, max_length=128) + message_index: int | None = Field(default=None, ge=0) + comment: str | None = Field(default=None, max_length=_MAX_COMMENT_LEN) + page_url: str | None = Field(default=None, max_length=2048) + + @field_validator("page_url") + @classmethod + def _validate_page_url_scheme(cls, url: str | None) -> str | None: + """Only accept http(s) URLs. + + This endpoint is anonymous, and page_url is later rendered as a clickable + link in the admin dashboard. Rejecting non-http(s) schemes prevents a + stored 'javascript:'/'data:' link from becoming XSS against an admin. + """ + if url is None: + return url + if not url.startswith(("http://", "https://")): + raise ValueError("page_url must start with http:// or https://") + return url + + @model_validator(mode="before") + @classmethod + def _normalize(cls, data: Any) -> Any: + """Normalize the raw input before field validation. + + Runs on the raw dict so the after-validator can be a pure guard: drop + any sentiment on general feedback (it is meaningless there) and collapse + a whitespace-only comment to None so the DB stays clean. + """ + if not isinstance(data, dict): + return data + if data.get("feedback_type") == "general": + data = {**data, "sentiment": None} + comment = data.get("comment") + if isinstance(comment, str) and not comment.strip(): + data = {**data, "comment": None} + return data + + @model_validator(mode="after") + def _check_shape(self) -> "FeedbackRequest": + """Enforce the per-type required fields (pure guard, no mutation).""" + if self.feedback_type == "response" and self.sentiment is None: + raise ValueError("response feedback requires a sentiment ('up' or 'down')") + if self.feedback_type == "general" and not (self.comment and self.comment.strip()): + raise ValueError("general feedback requires a non-empty comment") + return self + + +class FeedbackResponse(BaseModel): + """Acknowledgement returned to the widget.""" + + status: Literal["ok"] = "ok" + feedback_id: str + + +@router.post("/feedback", response_model=FeedbackResponse) +async def submit_feedback( + body: FeedbackRequest, + origin: Annotated[str | None, Header()] = None, +) -> FeedbackResponse: + """Record a piece of user feedback. + + Anonymous by design: widget users carry no API key. The community must be a + registered one. The Origin header is checked softly -- an unrecognized or + absent origin is logged (CLI, mobile, proxies strip it) but does not reject + the submission, mirroring how the chat endpoints degrade gracefully. + + Abuse/DoS defense: the public path to this endpoint is the Cloudflare Worker + (`POST /feedback` -> handleFeedback -> rateLimitOrReject), which rate-limits + per client before proxying here. There is intentionally no in-process limiter; + if this endpoint is ever exposed without the Worker in front, add one. + """ + info = registry.get(body.community_id) + if info is None: + raise HTTPException(status_code=404, detail=f"Unknown community: {body.community_id}") + + if not _is_authorized_origin(origin, body.community_id): + logger.info( + "Feedback from unrecognized origin %r for community %s (accepted)", + origin, + body.community_id, + ) + + entry = FeedbackEntry( + feedback_id=str(uuid.uuid4()), + timestamp=now_iso(), + community_id=body.community_id, + feedback_type=body.feedback_type, + sentiment=body.sentiment, + request_id=body.request_id, + session_id=body.session_id, + message_index=body.message_index, + comment=body.comment, + page_url=body.page_url, + ) + + # write_feedback is best-effort: it logs and swallows storage errors (and + # escalates after repeated failures) rather than failing the user's request, + # so the widget always receives a clean acknowledgement. + write_feedback(entry) + return FeedbackResponse(feedback_id=entry.feedback_id) diff --git a/src/api/routers/metrics.py b/src/api/routers/metrics.py index 467c28c..a022d28 100644 --- a/src/api/routers/metrics.py +++ b/src/api/routers/metrics.py @@ -14,6 +14,8 @@ from src.metrics.db import metrics_connection from src.metrics.queries import ( get_community_summary, + get_feedback_entries, + get_feedback_summary, get_overview, get_quality_summary, get_token_breakdown, @@ -71,6 +73,47 @@ async def token_breakdown( ) +@router.get("/feedback") +async def feedback( + auth: RequireScopedAuth, + community_id: str | None = Query(default=None, description="Filter by community"), + limit: int = Query(default=100, ge=1, le=500, description="Max comment rows to return"), + offset: int = Query(default=0, ge=0, description="Rows to skip (pagination)"), + comments_only: bool = Query( + default=False, description="Return only entries that carry a free-text comment" + ), +) -> dict[str, Any]: + """Get user feedback (thumbs up/down counts and free-text comments). + + Global admin keys can filter by any community (or see all). Per-community + keys are automatically scoped to their own community. + """ + # Community-scoped keys always filter to their own community + effective_community = community_id + if auth.role == "community": + effective_community = auth.community_id + + try: + with metrics_connection() as conn: + return { + "community_id": effective_community, + "summary": get_feedback_summary(conn, community_id=effective_community), + "entries": get_feedback_entries( + conn, + community_id=effective_community, + limit=limit, + offset=offset, + with_comment_only=comments_only, + ), + } + except sqlite3.Error: + logger.exception("Failed to query metrics database for feedback") + raise HTTPException( + status_code=503, + detail="Metrics database is temporarily unavailable.", + ) + + @router.get("/quality") async def quality_overview(auth: RequireScopedAuth) -> dict[str, Any]: """Get quality metrics overview. diff --git a/src/metrics/db.py b/src/metrics/db.py index 35b6af5..9e2b621 100644 --- a/src/metrics/db.py +++ b/src/metrics/db.py @@ -1,7 +1,7 @@ """Metrics storage layer using SQLite with WAL mode. -Single SQLite database at {data_dir}/metrics.db stores all request logs. -WAL mode enables concurrent reads during writes. +Single SQLite database at {data_dir}/metrics.db stores request logs and user +feedback. WAL mode enables concurrent reads during writes. """ import json @@ -12,6 +12,7 @@ from dataclasses import dataclass, field from datetime import UTC, datetime from pathlib import Path +from typing import Literal from langchain_core.messages import AIMessage, BaseMessage @@ -20,6 +21,14 @@ # Track consecutive log_request failures for escalation _log_request_failures: int = 0 +# Track consecutive write_feedback failures for escalation +_write_feedback_failures: int = 0 + +# Allowed feedback type / sentiment values. These mirror the SQLite CHECK +# constraints below and the FeedbackEntry / FeedbackRequest invariants. +FEEDBACK_TYPES = ("response", "general") +FEEDBACK_SENTIMENTS = ("up", "down") + METRICS_SCHEMA_SQL = """ CREATE TABLE IF NOT EXISTS request_log ( id INTEGER PRIMARY KEY AUTOINCREMENT, @@ -49,6 +58,27 @@ ON request_log(timestamp); CREATE INDEX IF NOT EXISTS idx_request_log_community_timestamp ON request_log(community_id, timestamp); + +CREATE TABLE IF NOT EXISTS feedback_log ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + feedback_id TEXT NOT NULL, + timestamp TEXT NOT NULL, + community_id TEXT NOT NULL, + feedback_type TEXT NOT NULL CHECK (feedback_type IN ('response', 'general')), + sentiment TEXT CHECK (sentiment IN ('up', 'down') OR sentiment IS NULL), + request_id TEXT, + session_id TEXT, + message_index INTEGER, + comment TEXT, + page_url TEXT +); + +CREATE UNIQUE INDEX IF NOT EXISTS idx_feedback_log_feedback_id + ON feedback_log(feedback_id); +CREATE INDEX IF NOT EXISTS idx_feedback_log_community + ON feedback_log(community_id); +CREATE INDEX IF NOT EXISTS idx_feedback_log_community_timestamp + ON feedback_log(community_id, timestamp); """ # Columns added after initial schema; ALTER TABLE for existing databases @@ -83,6 +113,48 @@ class RequestLogEntry: langfuse_trace_id: str | None = None +@dataclass +class FeedbackEntry: + """A single user-feedback entry for the metrics database. + + Two flavors share one table: + + * ``feedback_type="response"`` -- a thumbs up/down on a specific assistant + reply. ``sentiment`` is required; ``request_id`` (and/or ``message_index``) + links it back to the answer it rates. + * ``feedback_type="general"`` -- free-text feedback not tied to a single + reply. ``sentiment`` is typically ``None`` and ``comment`` carries the text. + """ + + feedback_id: str + timestamp: str + community_id: str + feedback_type: Literal["response", "general"] + sentiment: Literal["up", "down"] | None = None + request_id: str | None = None + session_id: str | None = None + message_index: int | None = None + comment: str | None = None + page_url: str | None = None + + def __post_init__(self) -> None: + """Enforce the storage-layer invariants. + + ``write_feedback`` is a public function callable outside the API router, + so this is the last checkpoint before a row reaches SQLite. Reject the + illegal shapes the satisfaction-rate query would otherwise be corrupted + by (a 'response' with no sentiment, or a 'general' carrying one). + """ + if self.feedback_type not in FEEDBACK_TYPES: + raise ValueError(f"feedback_type must be one of {FEEDBACK_TYPES!r}") + if self.sentiment is not None and self.sentiment not in FEEDBACK_SENTIMENTS: + raise ValueError(f"sentiment must be one of {FEEDBACK_SENTIMENTS!r} or None") + if self.feedback_type == "response" and self.sentiment is None: + raise ValueError("response feedback requires a sentiment") + if self.feedback_type == "general" and self.sentiment is not None: + raise ValueError("general feedback must not carry a sentiment") + + def get_metrics_db_path() -> Path: """Return path to the metrics SQLite database. @@ -230,6 +302,67 @@ def log_request(entry: RequestLogEntry, db_path: Path | None = None) -> None: conn.close() +def write_feedback(entry: FeedbackEntry, db_path: Path | None = None) -> None: + """Insert a feedback entry into the database. + + Mirrors :func:`log_request`: best-effort, never raises to the caller, and + escalates to a CRITICAL log after repeated failures so a broken disk or DB + surfaces in monitoring instead of silently dropping feedback. + + Args: + entry: The feedback entry to insert. + db_path: Optional path override (for testing). + """ + global _write_feedback_failures + conn: sqlite3.Connection | None = None + try: + # Acquire the connection inside the try so a connect failure (e.g. the + # data dir vanished) is handled here too and the function honors its + # "never raises" contract. + conn = get_metrics_connection(db_path) + conn.execute( + """ + INSERT INTO feedback_log ( + feedback_id, timestamp, community_id, feedback_type, sentiment, + request_id, session_id, message_index, comment, page_url + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + """, + ( + entry.feedback_id, + entry.timestamp, + entry.community_id, + entry.feedback_type, + entry.sentiment, + entry.request_id, + entry.session_id, + entry.message_index, + entry.comment, + entry.page_url, + ), + ) + conn.commit() + except sqlite3.Error: + _write_feedback_failures += 1 + logger.exception( + "Failed to write feedback %s (community=%s, type=%s) [failure #%d]", + entry.feedback_id, + entry.community_id, + entry.feedback_type, + _write_feedback_failures, + ) + if _write_feedback_failures >= 10: + logger.critical( + "Feedback DB write has failed %d times. " + "Possible disk/database issue requiring investigation.", + _write_feedback_failures, + ) + else: + _write_feedback_failures = 0 + finally: + if conn is not None: + conn.close() + + def extract_token_usage(result: dict) -> tuple[int, int, int]: """Extract token usage from agent result messages. diff --git a/src/metrics/queries.py b/src/metrics/queries.py index 8c2d7be..fae34b8 100644 --- a/src/metrics/queries.py +++ b/src/metrics/queries.py @@ -593,6 +593,122 @@ def get_quality_summary(community_id: str, conn: sqlite3.Connection) -> dict[str } +# --------------------------------------------------------------------------- +# Feedback queries +# --------------------------------------------------------------------------- + + +def get_feedback_summary( + conn: sqlite3.Connection, + community_id: str | None = None, +) -> dict[str, Any]: + """Get aggregate feedback counts. + + Args: + conn: SQLite connection (with row_factory=sqlite3.Row). + community_id: Filter to a single community, or None for all communities. + + Returns: + Dict with thumbs_up, thumbs_down, response_total, general_total, + comment_total, and a satisfaction_rate (up / (up + down)). + """ + where = "" + params: tuple = () + if community_id: + where = "WHERE community_id = ?" + params = (community_id,) + + row = conn.execute( + f""" + SELECT + COUNT(CASE WHEN sentiment = 'up' THEN 1 END) as thumbs_up, + COUNT(CASE WHEN sentiment = 'down' THEN 1 END) as thumbs_down, + COUNT(CASE WHEN feedback_type = 'response' THEN 1 END) as response_total, + COUNT(CASE WHEN feedback_type = 'general' THEN 1 END) as general_total, + COUNT(CASE WHEN comment IS NOT NULL AND TRIM(comment) != '' THEN 1 END) + as comment_total + FROM feedback_log + {where} + """, + params, + ).fetchone() + + up = row["thumbs_up"] + down = row["thumbs_down"] + rated = up + down + + return { + "community_id": community_id, + "thumbs_up": up, + "thumbs_down": down, + "response_total": row["response_total"], + "general_total": row["general_total"], + "comment_total": row["comment_total"], + "satisfaction_rate": round(up / rated, 4) if rated > 0 else None, + } + + +def get_feedback_entries( + conn: sqlite3.Connection, + community_id: str | None = None, + limit: int = 100, + offset: int = 0, + with_comment_only: bool = False, +) -> list[dict[str, Any]]: + """Get individual feedback rows, most recent first. + + Args: + conn: SQLite connection. + community_id: Filter to a single community, or None for all communities. + limit: Maximum number of rows to return (clamped 1..500). + offset: Number of rows to skip (for pagination). + with_comment_only: If True, return only rows that carry a free-text comment. + + Returns: + List of dicts with timestamp, community_id, feedback_type, sentiment, + comment, request_id, session_id, message_index, page_url. + """ + limit = max(1, min(int(limit), 500)) + offset = max(0, int(offset)) + + clauses: list[str] = [] + params: list[Any] = [] + if community_id: + clauses.append("community_id = ?") + params.append(community_id) + if with_comment_only: + clauses.append("comment IS NOT NULL AND TRIM(comment) != ''") + where = ("WHERE " + " AND ".join(clauses)) if clauses else "" + + rows = conn.execute( + f""" + SELECT + timestamp, community_id, feedback_type, sentiment, + comment, request_id, session_id, message_index, page_url + FROM feedback_log + {where} + ORDER BY timestamp DESC, id DESC + LIMIT ? OFFSET ? + """, + (*params, limit, offset), + ).fetchall() + + return [ + { + "timestamp": r["timestamp"], + "community_id": r["community_id"], + "feedback_type": r["feedback_type"], + "sentiment": r["sentiment"], + "comment": r["comment"], + "request_id": r["request_id"], + "session_id": r["session_id"], + "message_index": r["message_index"], + "page_url": r["page_url"], + } + for r in rows + ] + + def _percentile(sorted_values: list[float], pct: float) -> float | None: """Compute percentile from a sorted list of values. diff --git a/tests/test_api/test_feedback_endpoint.py b/tests/test_api/test_feedback_endpoint.py new file mode 100644 index 0000000..b307cf8 --- /dev/null +++ b/tests/test_api/test_feedback_endpoint.py @@ -0,0 +1,339 @@ +"""Tests for the feedback API: POST /feedback and GET /metrics/feedback.""" + +import os + +import pytest +from fastapi.testclient import TestClient + +from src.api.main import app +from src.metrics.db import get_metrics_connection, init_metrics_db +from src.metrics.queries import get_feedback_summary + +ADMIN_KEY = "test-feedback-admin-key" +HED_KEY = "hed-community-key" + + +@pytest.fixture +def feedback_db(tmp_path): + """Isolated, empty metrics DB via DATA_DIR (real path resolution, no mocks). + + get_metrics_db_path() reads DATA_DIR, so pointing it at tmp_path redirects + every metrics code path (writes, reads, middleware) to a throwaway DB. + """ + os.environ["DATA_DIR"] = str(tmp_path) + init_metrics_db() + yield tmp_path / "metrics.db" + del os.environ["DATA_DIR"] + + +@pytest.fixture +def scoped_auth_env(): + """Admin key + per-community key for hed.""" + from src.api.config import get_settings + + os.environ["API_KEYS"] = ADMIN_KEY + os.environ["REQUIRE_API_AUTH"] = "true" + os.environ["COMMUNITY_ADMIN_KEYS"] = f"hed:{HED_KEY}" + get_settings.cache_clear() + yield + del os.environ["API_KEYS"] + del os.environ["REQUIRE_API_AUTH"] + del os.environ["COMMUNITY_ADMIN_KEYS"] + get_settings.cache_clear() + + +@pytest.fixture +def client(): + return TestClient(app) + + +@pytest.mark.usefixtures("feedback_db") +class TestSubmitFeedback: + """POST /feedback (anonymous, no auth).""" + + def test_thumbs_up(self, client): + resp = client.post( + "/feedback", + json={ + "community_id": "hed", + "feedback_type": "response", + "sentiment": "up", + "request_id": "req-1", + "session_id": "sess-1", + "message_index": 1, + }, + ) + assert resp.status_code == 200 + body = resp.json() + assert body["status"] == "ok" + assert body["feedback_id"] + + def test_thumbs_down_with_comment(self, client): + resp = client.post( + "/feedback", + json={ + "community_id": "hed", + "feedback_type": "response", + "sentiment": "down", + "comment": "missed the BIDS sidecar question", + }, + ) + assert resp.status_code == 200 + + def test_general_feedback(self, client): + resp = client.post( + "/feedback", + json={ + "community_id": "hed", + "feedback_type": "general", + "comment": "Please add more examples.", + }, + ) + assert resp.status_code == 200 + + def test_unknown_community_rejected(self, client): + resp = client.post( + "/feedback", + json={ + "community_id": "not-a-real-community", + "feedback_type": "response", + "sentiment": "up", + }, + ) + assert resp.status_code == 404 + + def test_response_without_sentiment_rejected(self, client): + resp = client.post( + "/feedback", + json={"community_id": "hed", "feedback_type": "response"}, + ) + assert resp.status_code == 422 + + def test_general_without_comment_rejected(self, client): + resp = client.post( + "/feedback", + json={"community_id": "hed", "feedback_type": "general"}, + ) + assert resp.status_code == 422 + + def test_oversized_comment_rejected(self, client): + resp = client.post( + "/feedback", + json={ + "community_id": "hed", + "feedback_type": "general", + "comment": "x" * 6000, + }, + ) + assert resp.status_code == 422 + + @pytest.mark.parametrize( + "bad_url", + [ + "javascript:alert(1)", + "data:text/html,", + "ftp://evil.example/x", + ], + ) + def test_non_http_page_url_rejected(self, client, bad_url): + # page_url is rendered as a link in the admin dashboard; only http(s) + # schemes are allowed to prevent stored XSS against admins. + resp = client.post( + "/feedback", + json={ + "community_id": "hed", + "feedback_type": "response", + "sentiment": "up", + "page_url": bad_url, + }, + ) + assert resp.status_code == 422 + + def test_general_sentiment_is_stripped(self, feedback_db, client): + # A general payload that smuggles a sentiment must be stored with NULL + # sentiment so it never counts as a thumbs-down. + resp = client.post( + "/feedback", + json={ + "community_id": "hed", + "feedback_type": "general", + "sentiment": "down", + "comment": "smuggled sentiment", + }, + ) + assert resp.status_code == 200 + conn = get_metrics_connection(feedback_db) + try: + row = conn.execute( + "SELECT sentiment FROM feedback_log WHERE feedback_type='general'" + ).fetchone() + summary = get_feedback_summary(conn, community_id="hed") + finally: + conn.close() + assert row["sentiment"] is None + assert summary["thumbs_down"] == 0 + + def test_http_page_url_accepted(self, client): + resp = client.post( + "/feedback", + json={ + "community_id": "hed", + "feedback_type": "response", + "sentiment": "up", + "page_url": "https://hedtags.org/page", + }, + ) + assert resp.status_code == 200 + + def test_persisted_and_readable(self, feedback_db, client): + client.post( + "/feedback", + json={"community_id": "hed", "feedback_type": "response", "sentiment": "up"}, + ) + conn = get_metrics_connection(feedback_db) + try: + summary = get_feedback_summary(conn, community_id="hed") + finally: + conn.close() + assert summary["thumbs_up"] == 1 + + def test_request_id_round_trips(self, feedback_db, client): + # The widget keys a vote on the request_id from the chat `done` event; + # the stored row must carry it so feedback joins back to request_log. + client.post( + "/feedback", + json={ + "community_id": "hed", + "feedback_type": "response", + "sentiment": "down", + "request_id": "req-xyz-123", + "message_index": 4, + }, + ) + conn = get_metrics_connection(feedback_db) + try: + row = conn.execute("SELECT request_id, message_index FROM feedback_log").fetchone() + finally: + conn.close() + assert row["request_id"] == "req-xyz-123" + assert row["message_index"] == 4 + + +@pytest.mark.usefixtures("feedback_db", "scoped_auth_env") +class TestReadFeedback: + """GET /metrics/feedback (scoped admin auth).""" + + def _seed(self, client): + client.post( + "/feedback", + json={"community_id": "hed", "feedback_type": "response", "sentiment": "up"}, + ) + client.post( + "/feedback", + json={ + "community_id": "eeglab", + "feedback_type": "response", + "sentiment": "down", + }, + ) + + def test_admin_sees_all(self, client): + self._seed(client) + resp = client.get("/metrics/feedback", headers={"X-API-Key": ADMIN_KEY}) + assert resp.status_code == 200 + data = resp.json() + assert data["summary"]["thumbs_up"] == 1 + assert data["summary"]["thumbs_down"] == 1 + + def test_admin_aggregates_across_communities(self, client): + # Two up votes in different communities must sum, proving the admin view + # is not silently scoped to one community. + client.post( + "/feedback", + json={"community_id": "hed", "feedback_type": "response", "sentiment": "up"}, + ) + client.post( + "/feedback", + json={"community_id": "eeglab", "feedback_type": "response", "sentiment": "up"}, + ) + resp = client.get("/metrics/feedback", headers={"X-API-Key": ADMIN_KEY}) + assert resp.json()["summary"]["thumbs_up"] == 2 + + def test_comments_only_filter(self, client): + client.post( + "/feedback", + json={"community_id": "hed", "feedback_type": "response", "sentiment": "up"}, + ) + client.post( + "/feedback", + json={"community_id": "hed", "feedback_type": "general", "comment": "great tool"}, + ) + resp = client.get( + "/metrics/feedback", + params={"comments_only": "true"}, + headers={"X-API-Key": HED_KEY}, + ) + entries = resp.json()["entries"] + assert len(entries) == 1 + assert entries[0]["comment"] == "great tool" + + def test_limit_above_max_rejected(self, client): + resp = client.get( + "/metrics/feedback", + params={"limit": 9999}, + headers={"X-API-Key": HED_KEY}, + ) + assert resp.status_code == 422 + + def test_offset_pagination(self, client): + for i in range(3): + client.post( + "/feedback", + json={ + "community_id": "hed", + "feedback_type": "general", + "comment": f"note {i}", + }, + ) + page1 = client.get( + "/metrics/feedback", + params={"limit": 2, "offset": 0}, + headers={"X-API-Key": HED_KEY}, + ).json()["entries"] + page2 = client.get( + "/metrics/feedback", + params={"limit": 2, "offset": 2}, + headers={"X-API-Key": HED_KEY}, + ).json()["entries"] + assert len(page1) == 2 + assert len(page2) == 1 + # No overlap between pages + seen = {e["comment"] for e in page1} | {e["comment"] for e in page2} + assert seen == {"note 0", "note 1", "note 2"} + + def test_community_key_scoped_to_own(self, client): + self._seed(client) + resp = client.get("/metrics/feedback", headers={"X-API-Key": HED_KEY}) + assert resp.status_code == 200 + data = resp.json() + # hed key must only see hed's up vote, not eeglab's down vote + assert data["community_id"] == "hed" + assert data["summary"]["thumbs_up"] == 1 + assert data["summary"]["thumbs_down"] == 0 + assert all(e["community_id"] == "hed" for e in data["entries"]) + + def test_community_key_cannot_override_filter(self, client): + self._seed(client) + # hed key asks for eeglab; must still be scoped to hed + resp = client.get( + "/metrics/feedback", + params={"community_id": "eeglab"}, + headers={"X-API-Key": HED_KEY}, + ) + data = resp.json() + assert data["community_id"] == "hed" + assert data["summary"]["thumbs_down"] == 0 + + def test_requires_auth(self, client): + resp = client.get("/metrics/feedback") + assert resp.status_code == 401 diff --git a/tests/test_metrics/test_feedback_db.py b/tests/test_metrics/test_feedback_db.py new file mode 100644 index 0000000..515c246 --- /dev/null +++ b/tests/test_metrics/test_feedback_db.py @@ -0,0 +1,320 @@ +"""Tests for the feedback storage layer (feedback_log table + queries).""" + +import logging + +import pytest + +import src.metrics.db as db_module +from src.metrics.db import ( + FeedbackEntry, + get_metrics_connection, + init_metrics_db, + now_iso, + write_feedback, +) +from src.metrics.queries import get_feedback_entries, get_feedback_summary + + +@pytest.fixture +def reset_feedback_counter(): + """Reset the module-global failure counter around tests that exercise it.""" + db_module._write_feedback_failures = 0 + yield + db_module._write_feedback_failures = 0 + + +@pytest.fixture +def feedback_db(tmp_path): + """Create a temporary metrics database with feedback rows.""" + db_path = tmp_path / "metrics.db" + init_metrics_db(db_path) + + entries = [ + FeedbackEntry( + feedback_id="f1", + timestamp="2025-01-15T10:00:00+00:00", + community_id="hed", + feedback_type="response", + sentiment="up", + request_id="r1", + session_id="s1", + message_index=0, + ), + FeedbackEntry( + feedback_id="f2", + timestamp="2025-01-15T11:00:00+00:00", + community_id="hed", + feedback_type="response", + sentiment="down", + request_id="r2", + session_id="s1", + message_index=2, + comment="answer was wrong about epochs", + ), + FeedbackEntry( + feedback_id="f3", + timestamp="2025-01-15T12:00:00+00:00", + community_id="hed", + feedback_type="general", + sentiment=None, + comment="love this assistant", + page_url="https://hedtags.org", + ), + # A different community, to prove scoping + FeedbackEntry( + feedback_id="f4", + timestamp="2025-01-15T13:00:00+00:00", + community_id="eeglab", + feedback_type="response", + sentiment="up", + ), + ] + for e in entries: + write_feedback(e, db_path=db_path) + + return db_path + + +class TestFeedbackTable: + """Schema creation for feedback_log.""" + + def test_table_created(self, tmp_path): + db_path = tmp_path / "metrics.db" + init_metrics_db(db_path) + conn = get_metrics_connection(db_path) + try: + tables = conn.execute( + "SELECT name FROM sqlite_master WHERE type='table' AND name='feedback_log'" + ).fetchall() + assert len(tables) == 1 + finally: + conn.close() + + def test_table_added_to_existing_db(self, tmp_path): + """A pre-existing DB without feedback_log gets the table on re-init. + + This proves the CREATE TABLE IF NOT EXISTS path auto-provisions the + new table on already-deployed databases without a bespoke migration. + Simulated by initializing the full schema, dropping feedback_log to + recreate an "old" DB, then re-running init. + """ + db_path = tmp_path / "metrics.db" + init_metrics_db(db_path) + + conn = get_metrics_connection(db_path) + conn.execute("DROP TABLE feedback_log") + conn.commit() + conn.close() + + # Re-init (mirrors a deploy bringing the new schema). + init_metrics_db(db_path) + + conn = get_metrics_connection(db_path) + try: + tables = conn.execute( + "SELECT name FROM sqlite_master WHERE type='table' AND name='feedback_log'" + ).fetchall() + assert len(tables) == 1 + finally: + conn.close() + + +class TestWriteFeedback: + """write_feedback() persistence.""" + + def test_round_trip(self, tmp_path): + db_path = tmp_path / "metrics.db" + init_metrics_db(db_path) + write_feedback( + FeedbackEntry( + feedback_id="x1", + timestamp=now_iso(), + community_id="hed", + feedback_type="response", + sentiment="up", + request_id="req-abc", + ), + db_path=db_path, + ) + conn = get_metrics_connection(db_path) + try: + row = conn.execute("SELECT * FROM feedback_log WHERE feedback_id = 'x1'").fetchone() + assert row["sentiment"] == "up" + assert row["request_id"] == "req-abc" + assert row["feedback_type"] == "response" + finally: + conn.close() + + @pytest.mark.usefixtures("reset_feedback_counter") + def test_failure_counter_increments_and_resets(self, tmp_path): + # A UNIQUE feedback_id collision makes the second write fail at INSERT. + # write_feedback must swallow it, bump the counter, then reset on success. + db_path = tmp_path / "metrics.db" + init_metrics_db(db_path) + dup = FeedbackEntry( + feedback_id="dup", + timestamp=now_iso(), + community_id="hed", + feedback_type="response", + sentiment="up", + ) + write_feedback(dup, db_path=db_path) + assert db_module._write_feedback_failures == 0 + write_feedback(dup, db_path=db_path) # duplicate -> swallowed failure + assert db_module._write_feedback_failures == 1 + write_feedback( + FeedbackEntry( + feedback_id="ok2", + timestamp=now_iso(), + community_id="hed", + feedback_type="response", + sentiment="up", + ), + db_path=db_path, + ) + assert db_module._write_feedback_failures == 0 + + @pytest.mark.usefixtures("reset_feedback_counter") + def test_failure_escalates_to_critical(self, tmp_path, caplog): + db_path = tmp_path / "metrics.db" + init_metrics_db(db_path) + dup = FeedbackEntry( + feedback_id="d", + timestamp=now_iso(), + community_id="hed", + feedback_type="response", + sentiment="up", + ) + write_feedback(dup, db_path=db_path) # first succeeds + with caplog.at_level(logging.CRITICAL): + for _ in range(10): + write_feedback(dup, db_path=db_path) # all collide + assert any(r.levelno == logging.CRITICAL for r in caplog.records) + + +class TestFeedbackEntryInvariants: + """FeedbackEntry.__post_init__ rejects illegal shapes at the storage layer.""" + + def test_response_requires_sentiment(self): + with pytest.raises(ValueError, match="response feedback requires a sentiment"): + FeedbackEntry( + feedback_id="a", + timestamp=now_iso(), + community_id="hed", + feedback_type="response", + sentiment=None, + ) + + def test_general_must_not_carry_sentiment(self): + with pytest.raises(ValueError, match="general feedback must not carry a sentiment"): + FeedbackEntry( + feedback_id="b", + timestamp=now_iso(), + community_id="hed", + feedback_type="general", + sentiment="up", + comment="x", + ) + + def test_bad_feedback_type_rejected(self): + with pytest.raises(ValueError, match="feedback_type must be"): + FeedbackEntry( + feedback_id="c", + timestamp=now_iso(), + community_id="hed", + feedback_type="bogus", + ) + + def test_bad_sentiment_rejected(self): + with pytest.raises(ValueError, match="sentiment must be"): + FeedbackEntry( + feedback_id="d", + timestamp=now_iso(), + community_id="hed", + feedback_type="response", + sentiment="meh", + ) + + +class TestFeedbackSummary: + """get_feedback_summary() aggregation.""" + + def test_counts_for_community(self, feedback_db): + conn = get_metrics_connection(feedback_db) + try: + summary = get_feedback_summary(conn, community_id="hed") + finally: + conn.close() + assert summary["thumbs_up"] == 1 + assert summary["thumbs_down"] == 1 + assert summary["response_total"] == 2 + assert summary["general_total"] == 1 + assert summary["comment_total"] == 2 # f2 (note) + f3 (general) + assert summary["satisfaction_rate"] == 0.5 + + def test_counts_all_communities(self, feedback_db): + conn = get_metrics_connection(feedback_db) + try: + summary = get_feedback_summary(conn, community_id=None) + finally: + conn.close() + assert summary["thumbs_up"] == 2 # hed f1 + eeglab f4 + assert summary["thumbs_down"] == 1 + + def test_satisfaction_rate_none_when_no_ratings(self, tmp_path): + db_path = tmp_path / "metrics.db" + init_metrics_db(db_path) + conn = get_metrics_connection(db_path) + try: + summary = get_feedback_summary(conn, community_id="hed") + finally: + conn.close() + assert summary["satisfaction_rate"] is None + + +class TestFeedbackEntries: + """get_feedback_entries() listing.""" + + def test_scoped_to_community(self, feedback_db): + conn = get_metrics_connection(feedback_db) + try: + rows = get_feedback_entries(conn, community_id="hed") + finally: + conn.close() + assert len(rows) == 3 + assert all(r["community_id"] == "hed" for r in rows) + # Most recent first + assert rows[0]["timestamp"] >= rows[-1]["timestamp"] + + def test_comments_only_filter(self, feedback_db): + conn = get_metrics_connection(feedback_db) + try: + rows = get_feedback_entries(conn, community_id="hed", with_comment_only=True) + finally: + conn.close() + assert len(rows) == 2 + assert all(r["comment"] for r in rows) + + def test_limit_floor_clamped(self, feedback_db): + # limit=0 must be clamped up to 1 (without clamping, SQL LIMIT 0 would + # return zero rows). Asserting a non-empty result tests the clamp itself. + conn = get_metrics_connection(feedback_db) + try: + rows = get_feedback_entries(conn, community_id="hed", limit=0) + finally: + conn.close() + assert len(rows) == 1 + + def test_offset_pagination(self, feedback_db): + # hed has 3 entries; page through them with limit=2. + conn = get_metrics_connection(feedback_db) + try: + page1 = get_feedback_entries(conn, community_id="hed", limit=2, offset=0) + page2 = get_feedback_entries(conn, community_id="hed", limit=2, offset=2) + finally: + conn.close() + assert len(page1) == 2 + assert len(page2) == 1 + stamps = {r["timestamp"] for r in page1} | {r["timestamp"] for r in page2} + # No overlap, full coverage of the 3 hed rows. + assert len(stamps) == 3 From 29405b2b3b0f672e560d27b6266cbb7cf7d46084 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 5 Jun 2026 02:35:42 +0000 Subject: [PATCH 07/39] Bump version to 0.8.4.dev3 --- src/version.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/version.py b/src/version.py index d7db3dc..8deae8a 100644 --- a/src/version.py +++ b/src/version.py @@ -1,7 +1,7 @@ """Version information for OSA.""" -__version__ = "0.8.4.dev2" -__version_info__ = (0, 8, 4, "dev2") +__version__ = "0.8.4.dev3" +__version_info__ = (0, 8, 4, "dev3") def get_version() -> str: From b296ebffb396c7f6dc8e9ae06a01841406da8d96 Mon Sep 17 00:00:00 2001 From: "Seyed (Yahya) Shirazi" Date: Thu, 4 Jun 2026 19:39:19 -0700 Subject: [PATCH 08/39] feat: community-admin scoped PR auto-merge (#285) * feat: community-admin scoped PR auto-merge New workflow approves + squash auto-merges a PR only when the author is in the target community's maintainers list (read from the BASE branch), the diff touches only src/assistants//**, and the PR does not edit the maintainers field. Uses a dedicated GitHub App token (not the CI PAT); pull_request_target with base-only checkout never runs PR-head code; required CI checks still gate the merge. Community-id is whitelisted before interpolation. Includes a setup/trust-model doc. Validated: YAML + every embedded bash step (bash -n). Closes #283 * fix: address PR #285 security review - permissions: add pull-requests:read (the eligibility step reads the PR file list with github.token; with explicit permissions the unspecified scope is 'none', which would 403 every PR). - Scope bypass fix: read the changed-file list from the paginated /pulls/{n}/files API and check BOTH .filename and .previous_filename. gh pr diff --name-only omitted a rename's OLD path (letting a file be moved from outside the community dir into it) and truncated at 300 files. - Injection hardening: pass user.login and head.sha via env vars instead of interpolating ${{ }} into the run-step shell body. - A config.yaml deleted/renamed-away is now caught (its old path appears in the file list, the head-config fetch 404s, and the PR is held for review). - Checkout: drop fetch-depth:0 (only BASE_SHA is needed); fix the comment. - Comment/doc accuracy: GITHUB_TOKEN approval restriction wording, the what-it-does step ordering (maintainers-field check before author check), and the setup-incomplete behavior wording. --- .context/community-admin-merge.md | 166 ++++++++ .../workflows/community-admin-pr-merge.yml | 386 ++++++++++++++++++ CLAUDE.md | 1 + 3 files changed, 553 insertions(+) create mode 100644 .context/community-admin-merge.md create mode 100644 .github/workflows/community-admin-pr-merge.yml diff --git a/.context/community-admin-merge.md b/.context/community-admin-merge.md new file mode 100644 index 0000000..5310f92 --- /dev/null +++ b/.context/community-admin-merge.md @@ -0,0 +1,166 @@ +# Community Admin Auto-Merge + +Lets a community maintainer auto-merge a pull request (PR) that touches ONLY +their own community's directory, without waiting on a human reviewer. The normal +required status checks (ruff + tests) still gate the actual merge, so this only +removes the human-approval step, not the CI gate. + +Workflow file: `.github/workflows/community-admin-pr-merge.yml` + +## What it does + +When a PR is opened, synchronized, reopened, or marked ready-for-review against +`develop` or `main`, the workflow: + +1. Reads the PR's changed-file list via the GitHub API. +2. Confirms every changed path is under a single `src/assistants//` tree + (one community, nothing outside it). +3. Reads the `maintainers:` list from the BASE branch copy of + `src/assistants//config.yaml`. +4. Confirms the PR does NOT change the `maintainers:` field itself (a covert + admin-list change is rejected before the author is even considered). +5. Confirms the PR author is in that list (case-insensitive). +6. If all checks pass: approves the PR and enables a SQUASH auto-merge using a + dedicated GitHub App token. The merge fires only after required checks pass. + +If any check fails, or anything is ambiguous, the workflow no-ops (logs a notice +and exits cleanly). The PR then follows the normal human-review path. Default +posture is "do not merge". + +## Trust model + +- Source of truth for authorization is the `maintainers:` list in the + community's `config.yaml`, read from the BASE branch (the already-merged, + trusted code), NEVER from the PR head. This prevents a PR from granting itself + merge rights by adding its author to the list in the same change. +- Scope is path-restricted to exactly one `src/assistants//` directory. + A PR that touches two communities, or touches anything outside a community + directory (including `src/version.py`, CI, top-level files), is ineligible. +- Edits to the `maintainers:` field are excluded from auto-merge. Changing who + holds admin power always requires human review. The workflow detects this by + parsing the base vs head copies of `config.yaml` and comparing the normalized + maintainers lists. +- The actual merge still requires all branch-protection required checks to be + green. Auto-merge only removes the manual-approval gate. +- The workflow uses `pull_request_target` so it has access to secrets even for + fork PRs, but it NEVER checks out or executes PR-head code. The only checkout + is the trusted base branch; the PR diff is read through the API only. The head + copy of `config.yaml` is fetched and parsed solely to compare the maintainers + field; it is never executed. + +## Token model + +All write actions (approve + merge + confirmation comment) use a SHORT-LIVED +installation token minted from a dedicated GitHub App via +`actions/create-github-app-token@v1`. This App is independent from the +`CI_ADMIN_TOKEN` personal access token (PAT) used by the version-automation +workflows. The workflow's own `GITHUB_TOKEN` is kept at `contents: read` only, +so the workflow has no standing write power; every write is attributable to the +App. + +## One-time setup (human) + +These steps are done once by a repo admin. Until they are complete, the workflow +evaluates every PR but skips the write steps (mint, approve, merge) for +ineligible PRs. If an *eligible* PR is opened before the secrets exist, the +token-mint step fails with an error (the PR is not merged). Finish all steps +before relying on it. + +### 1. Create the GitHub App + +GitHub -> Settings (your org `OpenScience-Collective`) -> Developer settings -> +GitHub Apps -> New GitHub App. + +- GitHub App name: e.g. `OSA Community Auto-Merge`. +- Homepage URL: the repo URL is fine (`https://github.com/OpenScience-Collective/osa`). +- Webhook: UNCHECK "Active". No webhook is needed; the Action invokes the App by + minting a token, the App does not need to receive events. +- Repository permissions (set ONLY these; leave everything else "No access"): + - Pull requests: Read and write (needed to approve and to enable auto-merge) + - Contents: Read and write (needed for the squash merge to write the commit) + - Metadata: Read-only (mandatory; GitHub sets this automatically) +- Organization permissions: none. +- Account permissions: none. +- "Where can this GitHub App be installed?": Only on this account. + +Create the App. Note the App ID shown on the App's settings page. + +### 2. Install the App on this repo + +From the App's settings page -> Install App -> install it on +`OpenScience-Collective`, and restrict it to the `osa` repository only +("Only select repositories" -> `osa`). + +### 3. Generate a private key + +On the App's settings page -> "Private keys" -> "Generate a private key". This +downloads a `.pem` file. Keep it secret; you will paste its full contents into a +repo secret in the next step. You cannot retrieve it again later, only generate +a new one. + +### 4. Add the two repo secrets + +Repo -> Settings -> Secrets and variables -> Actions -> New repository secret: + +- `COMMUNITY_MERGE_APP_ID` = the numeric App ID from step 1. +- `COMMUNITY_MERGE_APP_PRIVATE_KEY` = the full contents of the `.pem` file from + step 3 (including the `-----BEGIN ... KEY-----` / `-----END ... KEY-----` + lines). + +### 5. Enable "Allow auto-merge" on the repo + +The workflow uses `gh pr merge --auto`, which requires the repository-level +auto-merge feature to be turned on. Repo -> Settings -> General -> "Pull +Requests" section -> check "Allow auto-merge". Without this, the merge step +fails (the PR will still be approved, but it will not merge automatically). + +### 6. Let the App merge past branch protection + +Branch protection on `develop` and `main` is configured in the GitHub UI (not in +the repo). Make sure the App is allowed to merge: + +- Required status checks must stay ON (ruff + tests). The App does NOT bypass + these; auto-merge waits for them. Do not add the App to any + "allow specified actors to bypass required pull requests / status checks" + allowlist; it should merge through the same gate everyone else uses. +- If a branch has "Restrict who can push to matching branches" (a push/merge + allowlist) enabled, add the GitHub App to that allowlist so its merge is + permitted. If that restriction is not enabled, no action is needed. +- "Require approvals" can stay on; the App's approval satisfies it. If you use + "Dismiss stale approvals", note that a new push will dismiss the App's + approval and the workflow will re-approve on the next `synchronize` event. + +## How a community admin uses it + +1. Open a PR against `develop` (or `main`) that changes only files inside your + community directory, `src/assistants//` (config.yaml, tools.py, + prompts, logo, etc.). +2. Make sure you are a listed maintainer in that directory's `config.yaml`. +3. Do not change the `maintainers:` field in the same PR; that needs human + review. +4. The workflow approves the PR and enables squash auto-merge. The PR merges + automatically once ruff and the test suite pass. A confirmation comment is + posted (and updated in place on later pushes, not duplicated). + +If your PR is not eligible (touches more than one community, touches files +outside your directory, changes the maintainers list, or you are not a listed +maintainer), the workflow simply does nothing and the PR follows the normal +review process. + +## Security notes and limitations + +- The `maintainers:` list is hand-maintained in each `config.yaml`. There is no + sync with GitHub Teams or org membership; whoever is listed there is trusted to + merge changes to that community's directory. +- Adding or removing a maintainer is intentionally excluded from auto-merge and + always requires a human-reviewed PR. +- A brand-new community (no `config.yaml` on the base branch yet) cannot be + auto-merged, because there is no trusted maintainers list to authorize + against. The first PR introducing a community needs human review. +- A `config.yaml` that fails to parse (malformed YAML, missing or empty + maintainers, non-string entries) is treated as "not eligible". +- Auto-merging community changes into `main` will NOT cut a release. Releases + are triggered by changes to `src/version.py`, and community PRs are + path-restricted so they never touch it. +- The workflow uses `pull_request_target` but never runs PR-head code; see the + Trust model section and the comment block at the top of the workflow file. diff --git a/.github/workflows/community-admin-pr-merge.yml b/.github/workflows/community-admin-pr-merge.yml new file mode 100644 index 0000000..89e6d79 --- /dev/null +++ b/.github/workflows/community-admin-pr-merge.yml @@ -0,0 +1,386 @@ +name: Community Admin PR Auto-Merge + +# Lets a community maintainer auto-merge a PR that ONLY touches their own +# community's directory (src/assistants//**). When eligible, this workflow +# approves the PR and enables squash auto-merge so the normal required status +# checks (ruff + tests) still gate the actual merge. +# +# ----------------------------------------------------------------------------- +# Trigger choice: pull_request_target (NOT pull_request) +# ----------------------------------------------------------------------------- +# We need to approve + merge, which requires a privileged token. On a plain +# `pull_request` event from a fork, secrets are NOT available, so we could not +# mint the GitHub App token. `pull_request_target` runs in the context of the +# BASE repo and therefore HAS access to secrets even for fork PRs. +# +# The well-known danger of pull_request_target is that it can be tricked into +# executing attacker-controlled PR-head code with secrets in scope. We avoid +# that entirely: +# - We NEVER check out the PR head. The only checkout is the BASE branch +# (the trusted, already-reviewed code), and we never run anything from the +# PR. The PR diff is read via the GitHub API only. +# - Authorization (the maintainers list) is read from the BASE branch version +# of config.yaml, never from the PR head, so a PR cannot grant itself power. +# - If the PR's diff touches the `maintainers:` field of any config.yaml, the +# PR is ineligible and goes to human review. +# Given these constraints, pull_request_target is the correct and safe choice. +# +# ----------------------------------------------------------------------------- +# Token model +# ----------------------------------------------------------------------------- +# GITHUB_TOKEN cannot approve a PR (GitHub prohibits a workflow's own token from +# submitting a review on the PR that triggered it) and, more importantly, we +# deliberately keep its permissions minimal (read-only) so this workflow has no +# standing write power. All write actions (approve + +# merge) are performed with a SHORT-LIVED installation token minted from a +# dedicated GitHub App via actions/create-github-app-token. The App is the only +# identity allowed to merge here, and it is independent from the CI_ADMIN_TOKEN +# PAT used by the version-automation workflows. Required secrets: +# - COMMUNITY_MERGE_APP_ID +# - COMMUNITY_MERGE_APP_PRIVATE_KEY +# See .context/community-admin-merge.md for the one-time App setup. + +on: + pull_request_target: + types: [opened, synchronize, reopened, ready_for_review] + branches: [develop, main] + +# Minimal standing permissions. contents:read for the base checkout; the +# eligibility step reads the PR's file list with github.token, which needs +# pull-requests:read. Every WRITE (approve, merge, comment) happens through the +# GitHub App installation token minted below, NOT through GITHUB_TOKEN. +permissions: + contents: read + pull-requests: read + +# One in-flight evaluation per PR; cancel stale runs on rapid synchronize. +concurrency: + group: community-admin-merge-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + evaluate-and-merge: + # Skip drafts outright; they are not ready and ready_for_review will fire + # the real evaluation when the author marks the PR ready. + if: github.event.pull_request.draft == false + runs-on: ubuntu-latest + steps: + # Checkout the BASE branch ONLY. This is trusted, already-merged code. + # We never check out github.event.pull_request.head; the PR diff is read + # via the API in later steps. We only ever `git show` BASE_SHA:config.yaml, + # and `ref: base.sha` guarantees that commit is present locally. + - name: Checkout base branch (trusted) + uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.base.sha }} + persist-credentials: false + + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: '3.12' + + - name: Install dependencies + run: pip install pyyaml + + # ---------------------------------------------------------------------- + # Eligibility evaluation. This step writes ONE output: `eligible` + # (true/false) plus `community` and `reason` for logging/comments. It + # performs NO writes and uses only the read-only github.token to list the + # PR's changed files. Default posture is NOT eligible: any ambiguity, + # parse failure, or unexpected condition results in eligible=false. + # ---------------------------------------------------------------------- + - name: Evaluate eligibility + id: eval + # All PR-controlled values are passed via env (never interpolated into + # the script body) so a crafted login/sha/ref cannot inject shell. + env: + GH_TOKEN: ${{ github.token }} + PR_NUMBER: ${{ github.event.pull_request.number }} + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + AUTHOR_LOGIN: ${{ github.event.pull_request.user.login }} + REPO: ${{ github.repository }} + run: | + set -euo pipefail + + # Helper: emit outputs and exit "not eligible" cleanly (no-op, exit 0). + not_eligible() { + echo "::notice::Not eligible for auto-merge: $1" + { + echo "eligible=false" + echo "community=" + echo "reason=$1" + } >> "$GITHUB_OUTPUT" + exit 0 + } + + # 1. Get the PR's changed files from the GitHub API (metadata only; we + # never merge or execute PR-head code). We use the paginated + # /pulls/{n}/files endpoint, NOT `gh pr diff --name-only`, for two + # reasons that are load-bearing for security: + # - It emits BOTH `.filename` (new path) and `.previous_filename` + # (a rename's OLD path). Checking both prevents a rename that + # moves a file FROM outside the community dir INTO it (the old + # path would be out of scope) from slipping the scope check. + # - `--paginate` returns ALL files; `gh pr diff` truncates large + # PRs at 300 files, which could hide out-of-scope files past the + # cutoff. + mapfile -t CHANGED < <( + gh api --paginate "/repos/$REPO/pulls/$PR_NUMBER/files" \ + -q '.[] | (.filename, (.previous_filename // empty))' | sort -u + ) + + if [ "${#CHANGED[@]}" -eq 0 ]; then + not_eligible "PR changes zero files" + fi + + echo "Changed files:" + printf ' %s\n' "${CHANGED[@]}" + + # 2. Every changed path must match ^src/assistants//... and all + # paths must resolve to the SAME single community id. A PR spanning + # multiple communities, or touching anything outside a community + # dir, is not eligible. + COMMUNITY="" + for f in "${CHANGED[@]}"; do + # Defense in depth: git reports normalized repo-relative paths, but + # reject anything containing a `..` segment or a leading slash just + # in case, so a path can never escape the community subtree. + if [[ "$f" == *"/../"* || "$f" == "../"* || "$f" == *"/.." || "$f" == /* ]]; then + not_eligible "path contains a parent-directory or absolute segment: $f" + fi + + # Strict regex: literal prefix, one path segment (no slash, no `..`) + # as the id, then a slash and at least one more character (a real + # file under the dir, including files at the community root). + if [[ "$f" =~ ^src/assistants/([^/]+)/.+$ ]]; then + id="${BASH_REMATCH[1]}" + else + not_eligible "path outside a single community dir: $f" + fi + + if [ -z "$COMMUNITY" ]; then + COMMUNITY="$id" + elif [ "$COMMUNITY" != "$id" ]; then + not_eligible "PR spans multiple communities ($COMMUNITY, $id)" + fi + done + + if [ -z "$COMMUNITY" ]; then + not_eligible "could not determine a single community id" + fi + + # Defense in depth: the path regex allows any non-slash characters in + # the id. Constrain it to a safe identifier (lowercase letters, digits, + # hyphen, underscore) before it is interpolated into git refs / paths, + # so a crafted path can never carry shell or ref metacharacters here. + if ! [[ "$COMMUNITY" =~ ^[a-z0-9_-]+$ ]]; then + not_eligible "community id has unexpected characters: $COMMUNITY" + fi + echo "Target community: $COMMUNITY" + + CONFIG_PATH="src/assistants/$COMMUNITY/config.yaml" + + # 3. The BASE branch must actually define this community with a + # config.yaml. If the base has no such config (e.g. a brand-new + # community introduced by this very PR), there is no trusted + # maintainers list to authorize against -> human review. + if ! git cat-file -e "$BASE_SHA:$CONFIG_PATH" 2>/dev/null; then + not_eligible "no $CONFIG_PATH on base branch (new community needs human review)" + fi + + # 4. SECURITY: read the maintainers list ONLY from the BASE branch + # version of config.yaml. Never from the PR head. A parse failure + # (malformed YAML, missing/empty maintainers) means not eligible. + git show "$BASE_SHA:$CONFIG_PATH" > /tmp/base_config.yaml || \ + not_eligible "could not read base config.yaml" + + # Extract a normalized (lowercased, sorted, newline-joined) maintainers + # list. Exit code 2 from the helper = parse/shape failure. + python - "$COMMUNITY" <<'PY' > /tmp/base_maintainers.txt || PARSE_RC=$? + import sys, yaml + community = sys.argv[1] + try: + with open("/tmp/base_config.yaml") as fh: + data = yaml.safe_load(fh) + except Exception as exc: # noqa: BLE001 - any parse error => ineligible + sys.stderr.write(f"base config.yaml failed to parse: {exc}\n") + sys.exit(2) + if not isinstance(data, dict): + sys.stderr.write("base config.yaml is not a mapping\n") + sys.exit(2) + maint = data.get("maintainers") + if not isinstance(maint, list) or not maint: + sys.stderr.write("base config.yaml has no non-empty maintainers list\n") + sys.exit(2) + names = [] + for m in maint: + if not isinstance(m, str) or not m.strip(): + sys.stderr.write("maintainers entry is not a non-empty string\n") + sys.exit(2) + names.append(m.strip().lower()) + # Normalized, de-duplicated, sorted for stable comparison. + for n in sorted(set(names)): + print(n) + PY + PARSE_RC=${PARSE_RC:-0} + if [ "$PARSE_RC" -ne 0 ]; then + not_eligible "base config.yaml maintainers could not be parsed" + fi + + # 5. SECURITY: if config.yaml is among the changed files, ensure the + # PR does NOT modify the maintainers field. Changing who holds + # admin power must go through human review. Compare the normalized + # maintainers list of base vs head of config.yaml. + if printf '%s\n' "${CHANGED[@]}" | grep -qxF "$CONFIG_PATH"; then + echo "config.yaml is modified; verifying maintainers field is unchanged" + + # Fetch the HEAD version of config.yaml via the API (raw contents at + # the PR head sha, from $HEAD_SHA in env). We parse it ONLY to compare + # the maintainers field; we never execute it. A failure to fetch/parse + # the head config => not eligible (conservative default). This also + # catches a config.yaml that the PR deleted or renamed away (the fetch + # 404s), so admin-list removal cannot ride through. + if ! gh api \ + -H "Accept: application/vnd.github.raw+json" \ + "/repos/$REPO/contents/$CONFIG_PATH?ref=$HEAD_SHA" \ + > /tmp/head_config.yaml 2>/dev/null; then + not_eligible "could not read head config.yaml for maintainers diff" + fi + + python - "$COMMUNITY" <<'PY' > /tmp/head_maintainers.txt || HEAD_RC=$? + import sys, yaml + try: + with open("/tmp/head_config.yaml") as fh: + data = yaml.safe_load(fh) + except Exception as exc: # noqa: BLE001 - any parse error => ineligible + sys.stderr.write(f"head config.yaml failed to parse: {exc}\n") + sys.exit(2) + if not isinstance(data, dict): + sys.stderr.write("head config.yaml is not a mapping\n") + sys.exit(2) + maint = data.get("maintainers") + if not isinstance(maint, list): + sys.stderr.write("head config.yaml has no maintainers list\n") + sys.exit(2) + names = [] + for m in maint: + if not isinstance(m, str) or not m.strip(): + sys.stderr.write("head maintainers entry is not a non-empty string\n") + sys.exit(2) + names.append(m.strip().lower()) + for n in sorted(set(names)): + print(n) + PY + HEAD_RC=${HEAD_RC:-0} + if [ "$HEAD_RC" -ne 0 ]; then + not_eligible "head config.yaml maintainers could not be parsed" + fi + + if ! diff -q /tmp/base_maintainers.txt /tmp/head_maintainers.txt >/dev/null; then + not_eligible "PR modifies the maintainers field (needs human review)" + fi + echo "maintainers field unchanged; OK to proceed" + fi + + # 6. Verify the PR author is in the BASE-branch maintainers list. + # GitHub usernames are case-insensitive, so compare lowercased. + # AUTHOR_LOGIN comes from env (never interpolated into the script). + AUTHOR="$(printf '%s' "$AUTHOR_LOGIN" | tr '[:upper:]' '[:lower:]')" + echo "PR author (normalized): $AUTHOR" + if ! grep -qxF "$AUTHOR" /tmp/base_maintainers.txt; then + not_eligible "author '$AUTHOR' is not a maintainer of '$COMMUNITY'" + fi + + # All checks passed. + echo "::notice::PR #$PR_NUMBER is eligible for auto-merge (community=$COMMUNITY, author=$AUTHOR)" + { + echo "eligible=true" + echo "community=$COMMUNITY" + echo "reason=author is a maintainer; changes scoped to $COMMUNITY" + } >> "$GITHUB_OUTPUT" + + # ---------------------------------------------------------------------- + # Mint the GitHub App installation token. Only reached when eligible. + # Short-lived; scoped to this repo by the App installation. + # ---------------------------------------------------------------------- + - name: Mint GitHub App token + if: steps.eval.outputs.eligible == 'true' + id: app_token + uses: actions/create-github-app-token@v1 + with: + app-id: ${{ secrets.COMMUNITY_MERGE_APP_ID }} + private-key: ${{ secrets.COMMUNITY_MERGE_APP_PRIVATE_KEY }} + + # Approve the PR using the App token. A clear, attributable review body. + - name: Approve PR + if: steps.eval.outputs.eligible == 'true' + env: + GH_TOKEN: ${{ steps.app_token.outputs.token }} + REPO: ${{ github.repository }} + PR_NUMBER: ${{ github.event.pull_request.number }} + COMMUNITY: ${{ steps.eval.outputs.community }} + run: | + set -euo pipefail + gh api \ + --method POST \ + -H "Accept: application/vnd.github+json" \ + "/repos/$REPO/pulls/$PR_NUMBER/reviews" \ + -f event=APPROVE \ + -f body="Auto-approved by community-admin auto-merge: changes are scoped to the **$COMMUNITY** community directory and the author is a listed maintainer. Required status checks still gate the merge." + + # Enable squash auto-merge with the App token. Using --auto means the + # merge only happens once all required checks (ruff + tests) pass, so we + # never bypass CI. If the PR is already mergeable, GitHub merges + # immediately; otherwise it waits. + - name: Enable squash auto-merge + if: steps.eval.outputs.eligible == 'true' + env: + GH_TOKEN: ${{ steps.app_token.outputs.token }} + REPO: ${{ github.repository }} + PR_NUMBER: ${{ github.event.pull_request.number }} + run: | + set -euo pipefail + gh pr merge "$PR_NUMBER" --repo "$REPO" --auto --squash + + # Post (find-or-update) a confirmation comment so we do not spam a new + # comment on every synchronize event. Uses the App token's identity. + - name: Comment confirmation + if: steps.eval.outputs.eligible == 'true' + continue-on-error: true + uses: actions/github-script@v7 + with: + github-token: ${{ steps.app_token.outputs.token }} + script: | + const community = process.env.COMMUNITY; + const marker = ''; + const body = `${marker}\n` + + `Auto-merge enabled for the **${community}** community.\n\n` + + `This PR only touches \`src/assistants/${community}/\` and was opened by a ` + + `listed maintainer, so it has been approved and queued for **squash** auto-merge. ` + + `It will merge automatically once all required checks (ruff + tests) pass.`; + + const { data: comments } = await github.rest.issues.listComments({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.payload.pull_request.number, + }); + const existing = comments.find(c => c.body && c.body.includes(marker)); + if (existing) { + await github.rest.issues.updateComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: existing.id, + body, + }); + } else { + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.payload.pull_request.number, + body, + }); + } + env: + COMMUNITY: ${{ steps.eval.outputs.community }} diff --git a/CLAUDE.md b/CLAUDE.md index 472865b..f8be9aa 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -162,6 +162,7 @@ src/ - **.context/yaml_registry.md** - YAML-based community config (internal notes) - **.context/community_onboarding_review.md** - Onboarding gap analysis - **.context/local-testing-guide.md** - Quick local testing reference +- **.context/community-admin-merge.md** - Maintainer auto-merge for community-scoped PRs (setup + trust model) - **Existing configs to reference**: `src/assistants/hed/config.yaml`, `src/assistants/eeglab/config.yaml` ### Tool System From efaf5bfb28d0894659a72c4b33e43bcf86ec0010 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 5 Jun 2026 02:39:29 +0000 Subject: [PATCH 09/39] Bump version to 0.8.4.dev4 --- src/version.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/version.py b/src/version.py index 8deae8a..a9ce623 100644 --- a/src/version.py +++ b/src/version.py @@ -1,7 +1,7 @@ """Version information for OSA.""" -__version__ = "0.8.4.dev3" -__version_info__ = (0, 8, 4, "dev3") +__version__ = "0.8.4.dev4" +__version_info__ = (0, 8, 4, "dev4") def get_version() -> str: From 399f35731af1d457d619c18d43f4394f047914aa Mon Sep 17 00:00:00 2001 From: "Seyed (Yahya) Shirazi" Date: Thu, 4 Jun 2026 21:20:45 -0700 Subject: [PATCH 10/39] feat: optional 'what went wrong' box on thumbs-down (#287) * feat: optional 'what went wrong' box on thumbs-down Thumbs-down now reveals a small inline comment box (What went wrong? optional) with Send/Skip under that reply, instead of only Thanks!. Thumbs-up is unchanged (one-click count). The down-vote is deferred until the user sends or skips, and is flushed (committed bare) when they send a new message, reset, or close the widget, so leaving the box open never drops the vote. The optional comment rides on the same single response/down row (linked by request_id) -- no double count, no backend change. Optimistic with rollback + error on POST failure. Closes #286 * fix: address PR #287 review (confirm-then-commit feedback) - commitResponseFeedback now confirms before committing: feedbackCommitted (the 'Thanks!' state) is set only AFTER the POST succeeds, so a failure never leaves a false success in the UI or in localStorage. - A flush on send/reset/close is best-effort (interactive=false): it never writes errors into a tearing-down or hidden UI; a pending down stays pending and retries on the next flush. Interactive Send/Skip/up surface failures and keep the box (with the typed comment) for retry. - saveHistory strips transient flags (_feedbackCommitting/_feedbackJustOpened/ feedbackDraft) and never persists an unconfirmed vote. --- frontend/osa-chat-widget.js | 218 +++++++++++++++++++++++++++++++----- 1 file changed, 192 insertions(+), 26 deletions(-) diff --git a/frontend/osa-chat-widget.js b/frontend/osa-chat-widget.js index 0f8ae9a..1b1d5c5 100644 --- a/frontend/osa-chat-widget.js +++ b/frontend/osa-chat-widget.js @@ -574,6 +574,7 @@ .osa-message-feedback { display: flex; align-items: center; + flex-wrap: wrap; gap: 4px; margin-top: 6px; } @@ -629,6 +630,63 @@ margin-left: 4px; } + .osa-feedback-comment { + flex-basis: 100%; + display: flex; + flex-direction: column; + gap: 6px; + margin-top: 4px; + } + + .osa-feedback-comment-input { + width: 100%; + box-sizing: border-box; + resize: vertical; + min-height: 44px; + padding: 6px 8px; + font: inherit; + font-size: 13px; + color: var(--osa-text); + background: var(--osa-bg); + border: 1px solid var(--osa-border); + border-radius: 6px; + } + + .osa-feedback-comment-input:focus { + outline: none; + border-color: var(--osa-primary); + } + + .osa-feedback-comment-actions { + display: flex; + gap: 8px; + justify-content: flex-end; + } + + .osa-feedback-comment-actions button { + font: inherit; + font-size: 12px; + padding: 4px 10px; + border-radius: 6px; + cursor: pointer; + border: 1px solid var(--osa-border); + } + + .osa-feedback-skip { + background: transparent; + color: var(--osa-text-light); + } + + .osa-feedback-send { + background: var(--osa-primary); + color: #fff; + border-color: var(--osa-primary); + } + + .osa-feedback-send:hover { + background: var(--osa-primary-dark); + } + .osa-suggestions { padding: 12px 16px; border-top: 1px solid var(--osa-border); @@ -1385,7 +1443,15 @@ } try { - const data = JSON.stringify({ messages, sessionId }); + // Persist only durable feedback state: drop transient flags and the + // in-progress draft, and never persist a vote that has not been confirmed + // by the server (so a reload can't show a false "recorded" state). + const persistable = messages.map((m) => { + const { _feedbackCommitting, _feedbackJustOpened, feedbackDraft, ...rest } = m; + if (rest.feedback && !rest.feedbackCommitted) delete rest.feedback; + return rest; + }); + const data = JSON.stringify({ messages: persistable, sessionId }); localStorage.setItem(CONFIG.storageKey, data); saveErrorShown = false; } catch (e) { @@ -1881,42 +1947,91 @@ } } - // Record a thumbs up/down on a specific assistant reply. One vote per reply - // per browser session (stored in localStorage): once recorded, the choice is - // locked in this browser to keep counts honest. The button updates optimistically - // for responsiveness, but if the POST fails the vote is rolled back so the user - // is not misled into thinking it was saved. - async function submitResponseFeedback(container, msgIndex, sentiment) { + // Select a thumbs up/down on a specific assistant reply. One vote per reply + // per browser session (stored in localStorage). Thumbs-up commits immediately; + // thumbs-down reveals an optional "what went wrong?" box and commits when the + // user sends/skips (or when the vote is flushed on send/reset/close). + function submitResponseFeedback(container, msgIndex, sentiment) { const msg = messages[msgIndex]; if (!msg || msg.role !== 'assistant') return; if (msg.feedback) return; // already voted on this reply if (sentiment !== 'up' && sentiment !== 'down') return; - // Optimistic update. msg.feedback = sentiment; - renderMessages(container); + if (sentiment === 'down') { + // Defer the post; reveal the optional comment box first. + msg._feedbackJustOpened = true; + renderMessages(container); + } else { + renderMessages(container); // show the selection immediately + commitResponseFeedback(container, msgIndex, { interactive: true }); + } + } + + // Post a per-response vote (with the optional down-vote comment) exactly once. + // Confirm-then-commit: the "Thanks!" / committed state is only shown AFTER the + // POST succeeds, so a failure never leaves a false success (in the UI or in + // localStorage). interactive=true (Send/Skip/up click) surfaces failures so the + // user can retry; interactive=false (a flush on send/reset/close) is best-effort + // and never writes to the UI, since the conversation may be mid-teardown. + async function commitResponseFeedback(container, msgIndex, { interactive = false } = {}) { + const msg = messages[msgIndex]; + if (!msg || !msg.feedback) return; + if (msg.feedbackCommitted || msg._feedbackCommitting) return; + msg._feedbackCommitting = true; + + const sentiment = msg.feedback; + const comment = (msg.feedbackDraft || '').trim(); const ok = await postFeedback({ feedback_type: 'response', sentiment, + comment: comment || null, request_id: msg.requestId || null, session_id: sessionId || null, message_index: msgIndex, }); + msg._feedbackCommitting = false; - if (!ok) { - // Roll back so the vote can be retried and the user knows it did not save. - delete msg.feedback; + if (ok) { + msg.feedbackCommitted = true; + delete msg.feedbackDraft; + delete msg._feedbackJustOpened; + // Reveal "Thanks!" (and replace any open box). Safe in every path: harmless + // on a hidden window, and a no-op for a reply already removed by a reset. renderMessages(container); - showError(container, 'Could not send feedback. Please try again.'); + try { + saveHistory(); + } catch (e) { + console.error('[OSA] Failed to persist feedback locally:', e); + } return; } - try { - saveHistory(); - } catch (e) { - console.error('[OSA] Failed to persist feedback locally:', e); + // Failed. An up-vote reverts to unvoted; a down-vote keeps its pending box + // (and the typed comment) so it can be retried on the next Send or flush. + if (sentiment === 'up') delete msg.feedback; + if (!interactive) { + // Best-effort flush during teardown: do not touch the (possibly hidden or + // already-reset) UI. A pending down stays pending and retries next flush. + console.warn('[OSA] Feedback flush did not send; will retry on next attempt.'); + return; } + if (sentiment === 'down') msg._feedbackJustOpened = true; // refocus the box + renderMessages(container); + showError(container, 'Could not send feedback. Please try again.'); + } + + // Commit any pending (down) vote whose comment box is still open, so leaving + // it open and then sending/resetting/closing never silently drops the vote. + // Best-effort (interactive=false): failures are not surfaced into a tearing-down UI. + function flushPendingResponseFeedback(container) { + messages.forEach((msg, idx) => { + if (msg && msg.role === 'assistant' && msg.feedback + && !msg.feedbackCommitted && !msg._feedbackCommitting) { + commitResponseFeedback(container, idx, { interactive: false }); + } + }); } // Open the general (free-text) feedback modal @@ -2300,17 +2415,34 @@ : ''; // Per-response feedback (thumbs up/down) for assistant replies, but not - // the canned opening greeting (index 0). Records sentiment only; free-text - // feedback lives in the "Send feedback" link in the footer. + // the canned opening greeting (index 0). Up is a one-click count; down + // reveals an optional "what went wrong?" box before it is committed. const showFeedback = msg.role === 'assistant' && msgIndex > 0; const fb = msg.feedback; - const feedbackRow = showFeedback - ? `
- - - -
` - : ''; + const committed = !!msg.feedbackCommitted; + const pendingDown = fb === 'down' && !committed; + let feedbackRow = ''; + if (showFeedback) { + const upBtn = ``; + const downBtn = ``; + if (pendingDown) { + feedbackRow = `
+ ${upBtn}${downBtn} +
+ +
+ + +
+
+
`; + } else { + feedbackRow = `
+ ${upBtn}${downBtn} + +
`; + } + } msgEl.innerHTML = `
@@ -2361,6 +2493,33 @@ }); }); + // Optional comment box for a pending thumbs-down + messagesEl.querySelectorAll('.osa-message-feedback .osa-feedback-comment').forEach(box => { + const row = box.closest('.osa-message-feedback'); + const msgIndex = parseInt(row.getAttribute('data-msg-index'), 10); + const textarea = box.querySelector('.osa-feedback-comment-input'); + if (textarea) { + textarea.addEventListener('input', () => { + if (messages[msgIndex]) messages[msgIndex].feedbackDraft = textarea.value; + }); + // Focus once, right after the box first appears (not on every re-render). + if (messages[msgIndex] && messages[msgIndex]._feedbackJustOpened) { + messages[msgIndex]._feedbackJustOpened = false; + textarea.focus(); + } + } + box.querySelector('.osa-feedback-send')?.addEventListener('click', (e) => { + e.stopPropagation(); + if (textarea && messages[msgIndex]) messages[msgIndex].feedbackDraft = textarea.value; + commitResponseFeedback(container, msgIndex, { interactive: true }); + }); + box.querySelector('.osa-feedback-skip')?.addEventListener('click', (e) => { + e.stopPropagation(); + if (messages[msgIndex]) messages[msgIndex].feedbackDraft = ''; + commitResponseFeedback(container, msgIndex, { interactive: true }); + }); + }); + if (isLoading) { const loadingEl = document.createElement('div'); loadingEl.className = 'osa-loading'; @@ -2630,6 +2789,9 @@ async function sendMessage(container, question) { if (isLoading || !question.trim()) return; + // Commit any open thumbs-down comment box before the conversation moves on. + flushPendingResponseFeedback(container); + isLoading = true; // Track message indices to avoid corruption on error @@ -2859,6 +3021,8 @@ // Reset chat function resetChat(container) { if (messages.length <= 1 || isLoading) return; + // Commit any open thumbs-down comment before the history is cleared. + flushPendingResponseFeedback(container); messages = [{ role: 'assistant', content: CONFIG.initialMessage }]; sessionId = null; // Clear session to start fresh on next message try { @@ -2886,6 +3050,8 @@ // Hide tooltip when chat opens if (tooltip) tooltip.classList.remove('visible'); } else { + // Commit any open thumbs-down comment box on close. + flushPendingResponseFeedback(container); chatWindow.classList.remove('open'); container.classList.remove('chat-open'); button.innerHTML = ICONS.chat; From 1d6e613cfd56442235d0b0e0a171d9c7c0fb375d Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 5 Jun 2026 04:21:00 +0000 Subject: [PATCH 11/39] Bump version to 0.8.4.dev5 --- src/version.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/version.py b/src/version.py index a9ce623..6ca4244 100644 --- a/src/version.py +++ b/src/version.py @@ -1,7 +1,7 @@ """Version information for OSA.""" -__version__ = "0.8.4.dev4" -__version_info__ = (0, 8, 4, "dev4") +__version__ = "0.8.4.dev5" +__version_info__ = (0, 8, 4, "dev5") def get_version() -> str: From 52a0273e4af578cfbb475619108a674a4509a0ff Mon Sep 17 00:00:00 2001 From: "Seyed (Yahya) Shirazi" Date: Thu, 4 Jun 2026 21:36:00 -0700 Subject: [PATCH 12/39] fix(widget): move Send feedback to the disclaimer line (#289) The footer line ('Send feedback . Powered by OSA vX') overflowed the window next to the page-URL toggle. Shorten the default disclaimer to 'This is an AI assistant and may make mistakes.' and move the Send feedback link to the end of that line (flex space-between). The footer keeps only 'Powered by OSA vX'. Closes #288 --- frontend/osa-chat-widget.js | 56 ++++++++++++++++++++----------------- 1 file changed, 30 insertions(+), 26 deletions(-) diff --git a/frontend/osa-chat-widget.js b/frontend/osa-chat-widget.js index 1b1d5c5..7065e82 100644 --- a/frontend/osa-chat-widget.js +++ b/frontend/osa-chat-widget.js @@ -53,7 +53,7 @@ pageContextLabel: 'Share page URL to help answer questions', // AI disclaimer shown above the footer disclaimerEnabled: true, - disclaimerText: 'This is a multi-agent AI assistant and may make mistakes. Please verify responses.', + disclaimerText: 'This is an AI assistant and may make mistakes.', disclaimerColor: '#9a3412', disclaimerBackground: '#fff7ed', // Fullscreen mode (for pop-out windows) @@ -866,14 +866,37 @@ } .osa-ai-disclaimer { - padding: 4px 16px; + padding: 5px 16px; font-size: 10px; - text-align: center; + display: flex; + align-items: center; + justify-content: space-between; + gap: 10px; border-top: 1px solid var(--osa-border); color: var(--osa-disclaimer-color, #9a3412); background: var(--osa-disclaimer-bg, #fff7ed); } + .osa-ai-disclaimer .osa-disclaimer-text { + text-align: left; + } + + .osa-ai-disclaimer .osa-feedback-link { + background: none; + border: none; + padding: 0; + font: inherit; + color: inherit; + cursor: pointer; + text-decoration: underline; + white-space: nowrap; + flex-shrink: 0; + } + + .osa-ai-disclaimer .osa-feedback-link:hover { + opacity: 0.75; + } + .osa-combined-footer { padding: 6px 16px; font-size: 11px; @@ -928,26 +951,6 @@ text-decoration: underline; } - .osa-combined-footer .osa-footer-powered .osa-feedback-link { - background: none; - border: none; - padding: 0; - font: inherit; - color: var(--osa-text-light); - cursor: pointer; - text-decoration: none; - } - - .osa-combined-footer .osa-footer-powered .osa-feedback-link:hover { - color: var(--osa-primary); - text-decoration: underline; - } - - .osa-combined-footer .osa-footer-powered .osa-footer-sep { - margin: 0 4px; - color: var(--osa-border); - } - .osa-feedback-textarea { resize: vertical; min-height: 80px; @@ -2276,15 +2279,16 @@ ${ICONS.send}
-
${escapeHtml(CONFIG.disclaimerText || '')}
+
+ ${escapeHtml(CONFIG.disclaimerText || '')} + +
- - · Powered by OSA
From b92a1d00ff79666806169efddeeaaf5d32d06dcb Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 5 Jun 2026 04:36:12 +0000 Subject: [PATCH 13/39] Bump version to 0.8.4.dev6 --- src/version.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/version.py b/src/version.py index 6ca4244..7648250 100644 --- a/src/version.py +++ b/src/version.py @@ -1,7 +1,7 @@ """Version information for OSA.""" -__version__ = "0.8.4.dev5" -__version_info__ = (0, 8, 4, "dev5") +__version__ = "0.8.4.dev6" +__version_info__ = (0, 8, 4, "dev6") def get_version() -> str: From e061c3c2f8520cd415df781b5cdb4054f4b2c5c8 Mon Sep 17 00:00:00 2001 From: "Seyed (Yahya) Shirazi" Date: Thu, 4 Jun 2026 22:09:49 -0700 Subject: [PATCH 14/39] fix(ci): pick community-admin merge method by target branch (#291) The community-admin auto-merge always used --squash, but the protect-main ruleset only allows merge-commits, so a community PR to main would fail at the merge step. Choose the method by base ref: --squash into develop (feature-branch convention) and --merge into main (release convention). Updates the confirmation comment, header comment, and setup doc to match. Closes #290 --- .context/community-admin-merge.md | 12 +++++---- .../workflows/community-admin-pr-merge.yml | 27 ++++++++++++------- 2 files changed, 25 insertions(+), 14 deletions(-) diff --git a/.context/community-admin-merge.md b/.context/community-admin-merge.md index 5310f92..0395034 100644 --- a/.context/community-admin-merge.md +++ b/.context/community-admin-merge.md @@ -20,8 +20,9 @@ When a PR is opened, synchronized, reopened, or marked ready-for-review against 4. Confirms the PR does NOT change the `maintainers:` field itself (a covert admin-list change is rejected before the author is even considered). 5. Confirms the PR author is in that list (case-insensitive). -6. If all checks pass: approves the PR and enables a SQUASH auto-merge using a - dedicated GitHub App token. The merge fires only after required checks pass. +6. If all checks pass: approves the PR and enables auto-merge using a dedicated + GitHub App token (squash into `develop`, merge-commit into `main`, matching + the branch rulesets). The merge fires only after required checks pass. If any check fails, or anything is ambiguous, the workflow no-ops (logs a notice and exits cleanly). The PR then follows the normal human-review path. Default @@ -77,7 +78,7 @@ GitHub Apps -> New GitHub App. minting a token, the App does not need to receive events. - Repository permissions (set ONLY these; leave everything else "No access"): - Pull requests: Read and write (needed to approve and to enable auto-merge) - - Contents: Read and write (needed for the squash merge to write the commit) + - Contents: Read and write (needed for the merge to write the commit) - Metadata: Read-only (mandatory; GitHub sets this automatically) - Organization permissions: none. - Account permissions: none. @@ -138,8 +139,9 @@ the repo). Make sure the App is allowed to merge: 2. Make sure you are a listed maintainer in that directory's `config.yaml`. 3. Do not change the `maintainers:` field in the same PR; that needs human review. -4. The workflow approves the PR and enables squash auto-merge. The PR merges - automatically once ruff and the test suite pass. A confirmation comment is +4. The workflow approves the PR and enables auto-merge (squash into develop, + merge-commit into main). The PR merges automatically once ruff and the test + suite pass. A confirmation comment is posted (and updated in place on later pushes, not duplicated). If your PR is not eligible (touches more than one community, touches files diff --git a/.github/workflows/community-admin-pr-merge.yml b/.github/workflows/community-admin-pr-merge.yml index 89e6d79..4aeac6f 100644 --- a/.github/workflows/community-admin-pr-merge.yml +++ b/.github/workflows/community-admin-pr-merge.yml @@ -2,8 +2,8 @@ name: Community Admin PR Auto-Merge # Lets a community maintainer auto-merge a PR that ONLY touches their own # community's directory (src/assistants//**). When eligible, this workflow -# approves the PR and enables squash auto-merge so the normal required status -# checks (ruff + tests) still gate the actual merge. +# approves the PR and enables auto-merge (squash into develop, merge-commit into +# main) so the normal required status checks (ruff + tests) still gate the merge. # # ----------------------------------------------------------------------------- # Trigger choice: pull_request_target (NOT pull_request) @@ -330,19 +330,28 @@ jobs: -f event=APPROVE \ -f body="Auto-approved by community-admin auto-merge: changes are scoped to the **$COMMUNITY** community directory and the author is a listed maintainer. Required status checks still gate the merge." - # Enable squash auto-merge with the App token. Using --auto means the - # merge only happens once all required checks (ruff + tests) pass, so we - # never bypass CI. If the PR is already mergeable, GitHub merges - # immediately; otherwise it waits. - - name: Enable squash auto-merge + # Enable auto-merge with the App token. --auto means the merge happens only + # once all required checks (ruff + tests) pass, so we never bypass CI. The + # merge method is chosen by target branch to satisfy the branch rulesets: + # develop allows squash (feature-branch convention); main is merge-commit + # only (the develop->main release convention). Using --squash on a main PR + # would be rejected by the protect-main ruleset. + - name: Enable auto-merge if: steps.eval.outputs.eligible == 'true' env: GH_TOKEN: ${{ steps.app_token.outputs.token }} REPO: ${{ github.repository }} PR_NUMBER: ${{ github.event.pull_request.number }} + BASE_REF: ${{ github.event.pull_request.base.ref }} run: | set -euo pipefail - gh pr merge "$PR_NUMBER" --repo "$REPO" --auto --squash + if [ "$BASE_REF" = "main" ]; then + METHOD="--merge" + else + METHOD="--squash" + fi + echo "Merging PR #$PR_NUMBER into $BASE_REF with $METHOD" + gh pr merge "$PR_NUMBER" --repo "$REPO" --auto "$METHOD" # Post (find-or-update) a confirmation comment so we do not spam a new # comment on every synchronize event. Uses the App token's identity. @@ -358,7 +367,7 @@ jobs: const body = `${marker}\n` + `Auto-merge enabled for the **${community}** community.\n\n` + `This PR only touches \`src/assistants/${community}/\` and was opened by a ` + - `listed maintainer, so it has been approved and queued for **squash** auto-merge. ` + + `listed maintainer, so it has been approved and queued for auto-merge. ` + `It will merge automatically once all required checks (ruff + tests) pass.`; const { data: comments } = await github.rest.issues.listComments({ From cc203ca00af6d7c2d1b61e49fbd97fff68789f91 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 5 Jun 2026 05:10:00 +0000 Subject: [PATCH 15/39] Bump version to 0.8.4.dev7 --- src/version.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/version.py b/src/version.py index 7648250..4969a6a 100644 --- a/src/version.py +++ b/src/version.py @@ -1,7 +1,7 @@ """Version information for OSA.""" -__version__ = "0.8.4.dev6" -__version_info__ = (0, 8, 4, "dev6") +__version__ = "0.8.4.dev7" +__version_info__ = (0, 8, 4, "dev7") def get_version() -> str: From 336feba787b58be2b36506badf10dd02a12fd227 Mon Sep 17 00:00:00 2001 From: "Seyed (Yahya) Shirazi" Date: Thu, 4 Jun 2026 22:25:05 -0700 Subject: [PATCH 16/39] chore: add neuromechanist as maintainer of HED, EEGLAB, NEMAR (#293) Grants the OSC owner scoped community-admin merge rights on these three communities and enables end-to-end testing of the auto-merge flow. Closes #292 --- src/assistants/eeglab/config.yaml | 1 + src/assistants/hed/config.yaml | 1 + src/assistants/nemar/config.yaml | 1 + 3 files changed, 3 insertions(+) diff --git a/src/assistants/eeglab/config.yaml b/src/assistants/eeglab/config.yaml index 7d147ff..62708c4 100644 --- a/src/assistants/eeglab/config.yaml +++ b/src/assistants/eeglab/config.yaml @@ -35,6 +35,7 @@ cors_origins: # Used for scoped admin access and budget alert @mentions maintainers: - arnodelorme + - neuromechanist # Budget limits for cost management budget: diff --git a/src/assistants/hed/config.yaml b/src/assistants/hed/config.yaml index 84a41f9..5ae8b89 100644 --- a/src/assistants/hed/config.yaml +++ b/src/assistants/hed/config.yaml @@ -33,6 +33,7 @@ openrouter_api_key_env_var: "OPENROUTER_API_KEY_HED" maintainers: - VisLab - yarikoptic + - neuromechanist # Budget limits for cost management budget: diff --git a/src/assistants/nemar/config.yaml b/src/assistants/nemar/config.yaml index 59df1a0..575316f 100644 --- a/src/assistants/nemar/config.yaml +++ b/src/assistants/nemar/config.yaml @@ -33,6 +33,7 @@ cors_origins: # Community maintainers (GitHub usernames) maintainers: - arnodelorme + - neuromechanist # Budget limits for cost management budget: From b6ee763f3d15a9bfe2c327455580afa269c84bdb Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 5 Jun 2026 05:25:19 +0000 Subject: [PATCH 17/39] Bump version to 0.8.4.dev8 --- src/version.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/version.py b/src/version.py index 4969a6a..e2f1555 100644 --- a/src/version.py +++ b/src/version.py @@ -1,7 +1,7 @@ """Version information for OSA.""" -__version__ = "0.8.4.dev7" -__version_info__ = (0, 8, 4, "dev7") +__version__ = "0.8.4.dev8" +__version_info__ = (0, 8, 4, "dev8") def get_version() -> str: From 19876522d5b919715b233530a33e1b0f28630ccf Mon Sep 17 00:00:00 2001 From: "Seyed (Yahya) Shirazi" Date: Thu, 4 Jun 2026 22:42:35 -0700 Subject: [PATCH 18/39] docs(hed): note community-admin scoped auto-merge for maintainers (#296) --- src/assistants/hed/config.yaml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/assistants/hed/config.yaml b/src/assistants/hed/config.yaml index 5ae8b89..c7d6562 100644 --- a/src/assistants/hed/config.yaml +++ b/src/assistants/hed/config.yaml @@ -462,3 +462,7 @@ extensions: - validate_hed_string - suggest_hed_tags - get_hed_schema_versions + +# Community maintainers listed above may open scoped PRs (touching only +# this directory) that auto-approve and auto-merge via the community-admin +# merge workflow. From a34604c819357c27bf620afd549bf01f9602ecb7 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 5 Jun 2026 05:42:46 +0000 Subject: [PATCH 19/39] Bump version to 0.8.4.dev9 --- src/version.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/version.py b/src/version.py index e2f1555..0741f2c 100644 --- a/src/version.py +++ b/src/version.py @@ -1,7 +1,7 @@ """Version information for OSA.""" -__version__ = "0.8.4.dev8" -__version_info__ = (0, 8, 4, "dev8") +__version__ = "0.8.4.dev9" +__version_info__ = (0, 8, 4, "dev9") def get_version() -> str: From 5730628e340c2c1b7ccb5e9096f4813d59bd0bd0 Mon Sep 17 00:00:00 2001 From: "Seyed (Yahya) Shirazi" Date: Thu, 4 Jun 2026 23:16:32 -0700 Subject: [PATCH 20/39] feat(ci): merge community PRs into develop on a maintainer's LGTM/merge comment (#298) * feat(ci): merge community PRs into develop on a maintainer's LGTM/merge comment Replace auto-merge-on-open with an explicit comment command: a community maintainer merges a scoped PR into develop by commenting both 'LGTM' and 'merge'. Triggers on issue_comment; authorizes the COMMENTER against the target community's base-branch maintainers; squash-merges pinned to the commented head (--match-head-commit) so a later push needs a fresh comment. Develop only -- an OSA admin merges develop to main. All prior guards keep: single-community path scope, maintainers-field-edit exclusion, community-id whitelist, never run PR-head code, App token for writes. Docs rewritten. Closes #297 * harden(ci): guard negated 'merge' comments + pin pyyaml Address PR #298 security review: reject comments that say 'do not merge'/ 'don't merge' even if they also contain LGTM+merge; pin pyyaml==6.0.2 in the privileged eval step. --- .context/community-admin-merge.md | 227 +++++------ .../workflows/community-admin-pr-merge.yml | 364 ++++++++---------- CLAUDE.md | 2 +- 3 files changed, 269 insertions(+), 324 deletions(-) diff --git a/.context/community-admin-merge.md b/.context/community-admin-merge.md index 0395034..282959f 100644 --- a/.context/community-admin-merge.md +++ b/.context/community-admin-merge.md @@ -1,53 +1,81 @@ -# Community Admin Auto-Merge +# Community Maintainer Merge Command -Lets a community maintainer auto-merge a pull request (PR) that touches ONLY -their own community's directory, without waiting on a human reviewer. The normal +Lets a community maintainer merge a pull request (PR) into `develop` that touches +ONLY their own community's directory, by leaving an explicit comment. Nothing +merges on open; the maintainer comments when they are ready to ship. The normal required status checks (ruff + tests) still gate the actual merge, so this only removes the human-approval step, not the CI gate. +**Scope is `develop` only.** Releases (`develop -> main`) are done by an OSA admin, +so `main` never diverges from the integrated `develop` branch. A community PR that +targets `main` is ignored by this workflow. + Workflow file: `.github/workflows/community-admin-pr-merge.yml` +## How a community maintainer uses it + +1. Open a PR against **`develop`** that changes only files inside your community + directory, `src/assistants//` (`config.yaml`, `tools.py`, prompts, + `logo.svg`, etc.). It does not have to be authored by you. +2. When you (a listed maintainer of that community) are ready to merge it, post a + PR comment containing both keywords **`LGTM`** and **`merge`**, for example: + + > LGTM, please merge + + Casing does not matter (`lgtm`/`LGTM`), and any phrasing works as long as both + words appear. +3. The bot approves the PR and enables **squash** auto-merge. The PR merges + automatically once ruff and the tests pass. A confirmation comment is posted + (and updated in place, not duplicated). + +That's it. If the PR is not eligible (see below), the comment simply does +nothing and the PR follows the normal review process. + ## What it does -When a PR is opened, synchronized, reopened, or marked ready-for-review against -`develop` or `main`, the workflow: +When a comment is posted on a PR, the workflow: -1. Reads the PR's changed-file list via the GitHub API. -2. Confirms every changed path is under a single `src/assistants//` tree - (one community, nothing outside it). -3. Reads the `maintainers:` list from the BASE branch copy of +1. Checks the comment contains both `LGTM` and `merge` (case-insensitive). If not, + it stops immediately. +2. Reads the PR via the GitHub API and confirms it is **open**, not a draft, and + targets **`develop`**. +3. Confirms every changed path (including a rename's old path) is under a single + `src/assistants//` tree (one community, nothing outside it). +4. Reads the `maintainers:` list from the **base-branch** copy of `src/assistants//config.yaml`. -4. Confirms the PR does NOT change the `maintainers:` field itself (a covert - admin-list change is rejected before the author is even considered). -5. Confirms the PR author is in that list (case-insensitive). -6. If all checks pass: approves the PR and enables auto-merge using a dedicated - GitHub App token (squash into `develop`, merge-commit into `main`, matching - the branch rulesets). The merge fires only after required checks pass. +5. Confirms the PR does NOT change the `maintainers:` field itself (a covert + admin-list change is excluded and needs human review). +6. Confirms the **commenter** is in that maintainers list (case-insensitive). +7. If all checks pass: approves the PR and enables squash auto-merge using a + dedicated GitHub App token, **pinned to the exact commit the comment was made + on**. The merge fires only after required checks pass. If any check fails, or anything is ambiguous, the workflow no-ops (logs a notice -and exits cleanly). The PR then follows the normal human-review path. Default -posture is "do not merge". +and exits cleanly). Default posture is "do not merge". ## Trust model -- Source of truth for authorization is the `maintainers:` list in the - community's `config.yaml`, read from the BASE branch (the already-merged, - trusted code), NEVER from the PR head. This prevents a PR from granting itself - merge rights by adding its author to the list in the same change. -- Scope is path-restricted to exactly one `src/assistants//` directory. - A PR that touches two communities, or touches anything outside a community - directory (including `src/version.py`, CI, top-level files), is ineligible. -- Edits to the `maintainers:` field are excluded from auto-merge. Changing who - holds admin power always requires human review. The workflow detects this by - parsing the base vs head copies of `config.yaml` and comparing the normalized - maintainers lists. -- The actual merge still requires all branch-protection required checks to be - green. Auto-merge only removes the manual-approval gate. -- The workflow uses `pull_request_target` so it has access to secrets even for - fork PRs, but it NEVER checks out or executes PR-head code. The only checkout - is the trusted base branch; the PR diff is read through the API only. The head - copy of `config.yaml` is fetched and parsed solely to compare the maintainers - field; it is never executed. +- **Authorization is the commenter**, who must be in the `maintainers:` list of + the target community's `config.yaml`, read from the **base branch** (the + already-merged, trusted code), NEVER from the PR head. This prevents a PR from + granting itself merge rights by adding someone to the list in the same change. +- **Scope** is path-restricted to exactly one `src/assistants//` directory. + A PR that touches two communities, or anything outside a community directory + (including `src/version.py`, CI, top-level files), is ineligible. +- **Maintainers-field edits are excluded.** Changing who holds merge power always + requires human review. The workflow parses the base vs head copies of + `config.yaml` and compares the normalized maintainers lists. +- **No approve-then-push bypass.** The merge is pinned to the head commit the + command was made on (`gh pr merge --match-head-commit`). If new commits are + pushed after the comment, the pinned auto-merge is cancelled and a fresh + `LGTM ... merge` comment is required. (The `protect-dev` ruleset does not + dismiss stale reviews on push, so the workflow enforces this itself.) +- **CI still gates.** The merge waits for all required checks (`--auto`); the App + does not bypass them. +- **`issue_comment` is privileged** (it runs in the base-repo context with + secrets, like `pull_request_target`), so the workflow NEVER checks out or runs + PR-head code. Everything is read through the API; the head copy of `config.yaml` + is fetched only to compare the maintainers field, never executed. ## Token model @@ -55,114 +83,95 @@ All write actions (approve + merge + confirmation comment) use a SHORT-LIVED installation token minted from a dedicated GitHub App via `actions/create-github-app-token@v1`. This App is independent from the `CI_ADMIN_TOKEN` personal access token (PAT) used by the version-automation -workflows. The workflow's own `GITHUB_TOKEN` is kept at `contents: read` only, -so the workflow has no standing write power; every write is attributable to the -App. +workflows. The workflow's own `GITHUB_TOKEN` is kept read-only, so the workflow +has no standing write power; every write is attributable to the App. ## One-time setup (human) -These steps are done once by a repo admin. Until they are complete, the workflow -evaluates every PR but skips the write steps (mint, approve, merge) for -ineligible PRs. If an *eligible* PR is opened before the secrets exist, the -token-mint step fails with an error (the PR is not merged). Finish all steps -before relying on it. +These steps are done once by a repo admin. Until they are complete, an +authorized `LGTM ... merge` comment will fail at the token-mint step (the PR is +not merged). Finish all steps before relying on it. ### 1. Create the GitHub App -GitHub -> Settings (your org `OpenScience-Collective`) -> Developer settings -> +GitHub -> Settings (org `OpenScience-Collective`) -> Developer settings -> GitHub Apps -> New GitHub App. - GitHub App name: e.g. `OSA Community Auto-Merge`. - Homepage URL: the repo URL is fine (`https://github.com/OpenScience-Collective/osa`). -- Webhook: UNCHECK "Active". No webhook is needed; the Action invokes the App by - minting a token, the App does not need to receive events. +- Webhook: UNCHECK "Active". No webhook is needed; the Action mints a token, the + App does not receive events. - Repository permissions (set ONLY these; leave everything else "No access"): - Pull requests: Read and write (needed to approve and to enable auto-merge) - - Contents: Read and write (needed for the merge to write the commit) + - Contents: Read and write (needed for the squash merge to write the commit) - Metadata: Read-only (mandatory; GitHub sets this automatically) -- Organization permissions: none. -- Account permissions: none. +- Organization permissions: none. Account permissions: none. - "Where can this GitHub App be installed?": Only on this account. Create the App. Note the App ID shown on the App's settings page. ### 2. Install the App on this repo -From the App's settings page -> Install App -> install it on -`OpenScience-Collective`, and restrict it to the `osa` repository only +From the App's settings page -> Install App -> install on +`OpenScience-Collective`, restricted to the `osa` repository only ("Only select repositories" -> `osa`). ### 3. Generate a private key On the App's settings page -> "Private keys" -> "Generate a private key". This -downloads a `.pem` file. Keep it secret; you will paste its full contents into a -repo secret in the next step. You cannot retrieve it again later, only generate -a new one. +downloads a `.pem` file. Keep it secret. You cannot retrieve it again later, only +generate a new one. ### 4. Add the two repo secrets Repo -> Settings -> Secrets and variables -> Actions -> New repository secret: - `COMMUNITY_MERGE_APP_ID` = the numeric App ID from step 1. -- `COMMUNITY_MERGE_APP_PRIVATE_KEY` = the full contents of the `.pem` file from - step 3 (including the `-----BEGIN ... KEY-----` / `-----END ... KEY-----` - lines). +- `COMMUNITY_MERGE_APP_PRIVATE_KEY` = the full contents of the `.pem` file + (including the `-----BEGIN ... KEY-----` / `-----END ... KEY-----` lines). + +CLI alternative for the secrets: + +```bash +gh secret set COMMUNITY_MERGE_APP_ID --repo OpenScience-Collective/osa --body "" +gh secret set COMMUNITY_MERGE_APP_PRIVATE_KEY --repo OpenScience-Collective/osa < /path/to/key.pem +``` ### 5. Enable "Allow auto-merge" on the repo The workflow uses `gh pr merge --auto`, which requires the repository-level -auto-merge feature to be turned on. Repo -> Settings -> General -> "Pull -Requests" section -> check "Allow auto-merge". Without this, the merge step -fails (the PR will still be approved, but it will not merge automatically). - -### 6. Let the App merge past branch protection - -Branch protection on `develop` and `main` is configured in the GitHub UI (not in -the repo). Make sure the App is allowed to merge: - -- Required status checks must stay ON (ruff + tests). The App does NOT bypass - these; auto-merge waits for them. Do not add the App to any - "allow specified actors to bypass required pull requests / status checks" - allowlist; it should merge through the same gate everyone else uses. -- If a branch has "Restrict who can push to matching branches" (a push/merge - allowlist) enabled, add the GitHub App to that allowlist so its merge is - permitted. If that restriction is not enabled, no action is needed. -- "Require approvals" can stay on; the App's approval satisfies it. If you use - "Dismiss stale approvals", note that a new push will dismiss the App's - approval and the workflow will re-approve on the next `synchronize` event. - -## How a community admin uses it - -1. Open a PR against `develop` (or `main`) that changes only files inside your - community directory, `src/assistants//` (config.yaml, tools.py, - prompts, logo, etc.). -2. Make sure you are a listed maintainer in that directory's `config.yaml`. -3. Do not change the `maintainers:` field in the same PR; that needs human - review. -4. The workflow approves the PR and enables auto-merge (squash into develop, - merge-commit into main). The PR merges automatically once ruff and the test - suite pass. A confirmation comment is - posted (and updated in place on later pushes, not duplicated). - -If your PR is not eligible (touches more than one community, touches files -outside your directory, changes the maintainers list, or you are not a listed -maintainer), the workflow simply does nothing and the PR follows the normal -review process. +auto-merge feature. Repo -> Settings -> General -> "Pull Requests" -> check +"Allow auto-merge". + +### 6. Registration: the workflow must live on the default branch (`main`) + +GitHub discovers/registers workflows from the **default branch** only. Because +this workflow triggers on `issue_comment`, the workflow file must be present on +`main` for comments to fire it. Keep `.github/workflows/community-admin-pr-merge.yml` +in sync on both `develop` and `main`. + +### 7. Branch protection / rulesets + +`develop` is protected by the `protect-dev` ruleset (required checks `Lint` + +`Test (3.12)`, 0 required reviews). The App merges *through* this gate (it waits +for the checks via `--auto`); it does NOT need to be added to any bypass list. If +you later enable "restrict who can push/merge" on `develop`, add the App to that +allowlist. ## Security notes and limitations -- The `maintainers:` list is hand-maintained in each `config.yaml`. There is no - sync with GitHub Teams or org membership; whoever is listed there is trusted to - merge changes to that community's directory. -- Adding or removing a maintainer is intentionally excluded from auto-merge and - always requires a human-reviewed PR. -- A brand-new community (no `config.yaml` on the base branch yet) cannot be - auto-merged, because there is no trusted maintainers list to authorize - against. The first PR introducing a community needs human review. -- A `config.yaml` that fails to parse (malformed YAML, missing or empty - maintainers, non-string entries) is treated as "not eligible". -- Auto-merging community changes into `main` will NOT cut a release. Releases - are triggered by changes to `src/version.py`, and community PRs are - path-restricted so they never touch it. -- The workflow uses `pull_request_target` but never runs PR-head code; see the - Trust model section and the comment block at the top of the workflow file. +- The `maintainers:` list is hand-maintained in each `config.yaml`; there is no + sync with GitHub Teams or org membership. Whoever is listed is trusted to merge + changes to that community's directory. +- The merge is **pinned to the commented commit**. A push after the `LGTM ... merge` + comment cancels the pending merge; a maintainer must re-comment. This is the + guard against getting an LGTM on a clean diff and then pushing changes. +- Adding/removing a maintainer, introducing a brand-new community (no base + `config.yaml` yet), or a `config.yaml` that fails to parse are all "not + eligible" and need a human-reviewed PR. +- A maintainer may comment on their **own** PR (GitHub allows self-comments, even + though it disallows self-approval), so a maintainer's own scoped PR is merged + when they comment the command. +- Community changes never reach `main` automatically; an OSA admin merges + `develop -> main` as part of a release. Community PRs are path-restricted and + never touch `src/version.py`, so they never cut a release on their own. diff --git a/.github/workflows/community-admin-pr-merge.yml b/.github/workflows/community-admin-pr-merge.yml index 4aeac6f..8e96c31 100644 --- a/.github/workflows/community-admin-pr-merge.yml +++ b/.github/workflows/community-admin-pr-merge.yml @@ -1,206 +1,174 @@ -name: Community Admin PR Auto-Merge +name: Community Maintainer Merge Command -# Lets a community maintainer auto-merge a PR that ONLY touches their own -# community's directory (src/assistants//**). When eligible, this workflow -# approves the PR and enables auto-merge (squash into develop, merge-commit into -# main) so the normal required status checks (ruff + tests) still gate the merge. +# A community maintainer merges a PR INTO develop that touches ONLY their own +# community's directory (src/assistants//**) by posting a comment that +# contains BOTH "LGTM" and "merge" (e.g. "LGTM, please merge"). Nothing merges on +# open -- the comment is the deliberate, explicit signal. (Chosen over GitHub +# "Approve", which reviewers give during review without intending to ship now.) +# +# Scope is develop only. Releases (develop -> main) are done by an OSA admin, so +# main never diverges from the integrated develop branch. # # ----------------------------------------------------------------------------- -# Trigger choice: pull_request_target (NOT pull_request) +# Trigger + security model # ----------------------------------------------------------------------------- -# We need to approve + merge, which requires a privileged token. On a plain -# `pull_request` event from a fork, secrets are NOT available, so we could not -# mint the GitHub App token. `pull_request_target` runs in the context of the -# BASE repo and therefore HAS access to secrets even for fork PRs. -# -# The well-known danger of pull_request_target is that it can be tricked into -# executing attacker-controlled PR-head code with secrets in scope. We avoid -# that entirely: -# - We NEVER check out the PR head. The only checkout is the BASE branch -# (the trusted, already-reviewed code), and we never run anything from the -# PR. The PR diff is read via the GitHub API only. +# issue_comment runs in the BASE-repo context and HAS access to secrets (so we +# can mint the GitHub App token), even for comments on fork PRs. It is therefore +# as privileged as pull_request_target, and we treat it the same way: +# - We NEVER check out or run PR-head code. Everything is read via the API. # - Authorization (the maintainers list) is read from the BASE branch version -# of config.yaml, never from the PR head, so a PR cannot grant itself power. -# - If the PR's diff touches the `maintainers:` field of any config.yaml, the -# PR is ineligible and goes to human review. -# Given these constraints, pull_request_target is the correct and safe choice. +# of config.yaml at the PR's base sha, never from the PR head, so a PR cannot +# grant itself power. +# - The PR must touch ONLY src/assistants//** (rename old paths +# checked too) and must NOT edit the maintainers field; the community id is +# whitelisted before it is used. +# - The COMMENTER must be a maintainer of that community. +# - The merge is pinned to the exact head commit the comment was made on +# (--match-head-commit), so a push AFTER the comment requires a fresh +# "LGTM ... merge". (protect-dev keeps stale approvals, so we pin ourselves.) +# - --auto means the merge waits for required checks; we never bypass CI. # -# ----------------------------------------------------------------------------- -# Token model -# ----------------------------------------------------------------------------- -# GITHUB_TOKEN cannot approve a PR (GitHub prohibits a workflow's own token from -# submitting a review on the PR that triggered it) and, more importantly, we -# deliberately keep its permissions minimal (read-only) so this workflow has no -# standing write power. All write actions (approve + -# merge) are performed with a SHORT-LIVED installation token minted from a -# dedicated GitHub App via actions/create-github-app-token. The App is the only -# identity allowed to merge here, and it is independent from the CI_ADMIN_TOKEN -# PAT used by the version-automation workflows. Required secrets: -# - COMMUNITY_MERGE_APP_ID -# - COMMUNITY_MERGE_APP_PRIVATE_KEY -# See .context/community-admin-merge.md for the one-time App setup. +# Token model: all writes (approve, merge, comment) use a SHORT-LIVED GitHub App +# installation token (secrets COMMUNITY_MERGE_APP_ID / COMMUNITY_MERGE_APP_PRIVATE_KEY), +# minted only when a command is authorized. GITHUB_TOKEN stays read-only. +# See .context/community-admin-merge.md for the one-time App setup + usage. on: - pull_request_target: - types: [opened, synchronize, reopened, ready_for_review] - branches: [develop, main] + issue_comment: + types: [created] -# Minimal standing permissions. contents:read for the base checkout; the -# eligibility step reads the PR's file list with github.token, which needs -# pull-requests:read. Every WRITE (approve, merge, comment) happens through the -# GitHub App installation token minted below, NOT through GITHUB_TOKEN. +# Minimal standing permissions; the eligibility step reads PR data with +# github.token (needs pull-requests:read). Every WRITE happens through the App +# installation token minted below, NOT through GITHUB_TOKEN. permissions: contents: read pull-requests: read -# One in-flight evaluation per PR; cancel stale runs on rapid synchronize. concurrency: - group: community-admin-merge-${{ github.event.pull_request.number }} - cancel-in-progress: true + group: community-maintainer-merge-${{ github.event.issue.number }} + cancel-in-progress: false jobs: - evaluate-and-merge: - # Skip drafts outright; they are not ready and ready_for_review will fire - # the real evaluation when the author marks the PR ready. - if: github.event.pull_request.draft == false + merge-on-command: + # Only comments on pull requests (issue_comment also fires on plain issues). + if: github.event.issue.pull_request != null runs-on: ubuntu-latest steps: - # Checkout the BASE branch ONLY. This is trusted, already-merged code. - # We never check out github.event.pull_request.head; the PR diff is read - # via the API in later steps. We only ever `git show` BASE_SHA:config.yaml, - # and `ref: base.sha` guarantees that commit is present locally. - - name: Checkout base branch (trusted) - uses: actions/checkout@v4 - with: - ref: ${{ github.event.pull_request.base.sha }} - persist-credentials: false + # Cheap first gate: does the comment carry the "LGTM ... merge" command? + # Case-insensitive, both keywords required. Skips the rest (and the Python + # setup) for ordinary comments. COMMENT_BODY via env (never interpolated). + - name: Detect command + id: cmd + env: + COMMENT_BODY: ${{ github.event.comment.body }} + run: | + set -euo pipefail + body="$(printf '%s' "$COMMENT_BODY" | tr '[:upper:]' '[:lower:]')" + # Guard the most explicit negations so "LGTM, but do not merge yet" + # does not fire. (Bounded anyway: maintainer-only, CI-gated, cancellable.) + negated=false + case "$body" in + *"do not merge"*|*"don't merge"*|*"dont merge"*) negated=true ;; + esac + if [[ "$body" == *lgtm* && "$body" == *merge* && "$negated" == false ]]; then + echo "command=true" >> "$GITHUB_OUTPUT" + echo "Detected LGTM/merge command." + else + echo "command=false" >> "$GITHUB_OUTPUT" + echo "No actionable LGTM/merge command (keywords absent or negated); nothing to do." + fi - name: Set up Python + if: steps.cmd.outputs.command == 'true' uses: actions/setup-python@v5 with: python-version: '3.12' - name: Install dependencies - run: pip install pyyaml + if: steps.cmd.outputs.command == 'true' + # Pinned: this step parses YAML in a privileged context (App token is + # minted only in a later step, but pin anyway for supply-chain hygiene). + run: pip install "pyyaml==6.0.2" # ---------------------------------------------------------------------- - # Eligibility evaluation. This step writes ONE output: `eligible` - # (true/false) plus `community` and `reason` for logging/comments. It - # performs NO writes and uses only the read-only github.token to list the - # PR's changed files. Default posture is NOT eligible: any ambiguity, - # parse failure, or unexpected condition results in eligible=false. + # Eligibility. Writes outputs: eligible (true/false), community, head_sha, + # method, commenter. Performs NO writes; uses only the read-only + # github.token. Default posture is NOT eligible. # ---------------------------------------------------------------------- - name: Evaluate eligibility id: eval - # All PR-controlled values are passed via env (never interpolated into - # the script body) so a crafted login/sha/ref cannot inject shell. + if: steps.cmd.outputs.command == 'true' env: GH_TOKEN: ${{ github.token }} - PR_NUMBER: ${{ github.event.pull_request.number }} - BASE_SHA: ${{ github.event.pull_request.base.sha }} - HEAD_SHA: ${{ github.event.pull_request.head.sha }} - AUTHOR_LOGIN: ${{ github.event.pull_request.user.login }} REPO: ${{ github.repository }} + PR_NUMBER: ${{ github.event.issue.number }} + COMMENTER_LOGIN: ${{ github.event.comment.user.login }} run: | set -euo pipefail - # Helper: emit outputs and exit "not eligible" cleanly (no-op, exit 0). not_eligible() { - echo "::notice::Not eligible for auto-merge: $1" - { - echo "eligible=false" - echo "community=" - echo "reason=$1" - } >> "$GITHUB_OUTPUT" + echo "::notice::Not merging: $1" + { echo "eligible=false"; echo "community="; } >> "$GITHUB_OUTPUT" exit 0 } - # 1. Get the PR's changed files from the GitHub API (metadata only; we - # never merge or execute PR-head code). We use the paginated - # /pulls/{n}/files endpoint, NOT `gh pr diff --name-only`, for two - # reasons that are load-bearing for security: - # - It emits BOTH `.filename` (new path) and `.previous_filename` - # (a rename's OLD path). Checking both prevents a rename that - # moves a file FROM outside the community dir INTO it (the old - # path would be out of scope) from slipping the scope check. - # - `--paginate` returns ALL files; `gh pr diff` truncates large - # PRs at 300 files, which could hide out-of-scope files past the - # cutoff. + # 1. Fetch the PR (data only; we never run its code). + PR_JSON="$(gh api "/repos/$REPO/pulls/$PR_NUMBER")" + BASE_REF="$(jq -r '.base.ref' <<<"$PR_JSON")" + BASE_SHA="$(jq -r '.base.sha' <<<"$PR_JSON")" + HEAD_SHA="$(jq -r '.head.sha' <<<"$PR_JSON")" + STATE="$(jq -r '.state' <<<"$PR_JSON")" + IS_DRAFT="$(jq -r '.draft' <<<"$PR_JSON")" + + [ "$STATE" = "open" ] || not_eligible "PR is not open ($STATE)" + [ "$IS_DRAFT" = "false" ] || not_eligible "PR is a draft" + # Community maintainers may merge into develop ONLY. Releases to main + # (develop -> main) are done by an OSA admin, so a community PR targeting + # main is never merged here (a main PR can also be stale vs develop). + [ "$BASE_REF" = "develop" ] || \ + not_eligible "base branch '$BASE_REF' is not develop (community PRs target develop; an OSA admin merges develop to main)" + + # 2. Changed files via the paginated /files API (both new and rename-old + # paths). Every path must be under ONE src/assistants// tree. mapfile -t CHANGED < <( gh api --paginate "/repos/$REPO/pulls/$PR_NUMBER/files" \ -q '.[] | (.filename, (.previous_filename // empty))' | sort -u ) + [ "${#CHANGED[@]}" -gt 0 ] || not_eligible "PR changes zero files" + echo "Changed files:"; printf ' %s\n' "${CHANGED[@]}" - if [ "${#CHANGED[@]}" -eq 0 ]; then - not_eligible "PR changes zero files" - fi - - echo "Changed files:" - printf ' %s\n' "${CHANGED[@]}" - - # 2. Every changed path must match ^src/assistants//... and all - # paths must resolve to the SAME single community id. A PR spanning - # multiple communities, or touching anything outside a community - # dir, is not eligible. COMMUNITY="" for f in "${CHANGED[@]}"; do - # Defense in depth: git reports normalized repo-relative paths, but - # reject anything containing a `..` segment or a leading slash just - # in case, so a path can never escape the community subtree. if [[ "$f" == *"/../"* || "$f" == "../"* || "$f" == *"/.." || "$f" == /* ]]; then not_eligible "path contains a parent-directory or absolute segment: $f" fi - - # Strict regex: literal prefix, one path segment (no slash, no `..`) - # as the id, then a slash and at least one more character (a real - # file under the dir, including files at the community root). if [[ "$f" =~ ^src/assistants/([^/]+)/.+$ ]]; then id="${BASH_REMATCH[1]}" else not_eligible "path outside a single community dir: $f" fi - if [ -z "$COMMUNITY" ]; then COMMUNITY="$id" elif [ "$COMMUNITY" != "$id" ]; then not_eligible "PR spans multiple communities ($COMMUNITY, $id)" fi done - - if [ -z "$COMMUNITY" ]; then - not_eligible "could not determine a single community id" - fi - - # Defense in depth: the path regex allows any non-slash characters in - # the id. Constrain it to a safe identifier (lowercase letters, digits, - # hyphen, underscore) before it is interpolated into git refs / paths, - # so a crafted path can never carry shell or ref metacharacters here. + [ -n "$COMMUNITY" ] || not_eligible "could not determine a single community id" if ! [[ "$COMMUNITY" =~ ^[a-z0-9_-]+$ ]]; then not_eligible "community id has unexpected characters: $COMMUNITY" fi echo "Target community: $COMMUNITY" - CONFIG_PATH="src/assistants/$COMMUNITY/config.yaml" - # 3. The BASE branch must actually define this community with a - # config.yaml. If the base has no such config (e.g. a brand-new - # community introduced by this very PR), there is no trusted - # maintainers list to authorize against -> human review. - if ! git cat-file -e "$BASE_SHA:$CONFIG_PATH" 2>/dev/null; then + # 3. Read the maintainers list ONLY from the BASE config (base sha), via + # the API. Never the PR head. Missing/unparseable -> not eligible. + if ! gh api -H "Accept: application/vnd.github.raw+json" \ + "/repos/$REPO/contents/$CONFIG_PATH?ref=$BASE_SHA" \ + > /tmp/base_config.yaml 2>/dev/null; then not_eligible "no $CONFIG_PATH on base branch (new community needs human review)" fi - - # 4. SECURITY: read the maintainers list ONLY from the BASE branch - # version of config.yaml. Never from the PR head. A parse failure - # (malformed YAML, missing/empty maintainers) means not eligible. - git show "$BASE_SHA:$CONFIG_PATH" > /tmp/base_config.yaml || \ - not_eligible "could not read base config.yaml" - - # Extract a normalized (lowercased, sorted, newline-joined) maintainers - # list. Exit code 2 from the helper = parse/shape failure. - python - "$COMMUNITY" <<'PY' > /tmp/base_maintainers.txt || PARSE_RC=$? + python - <<'PY' > /tmp/base_maintainers.txt || PARSE_RC=$? import sys, yaml - community = sys.argv[1] try: with open("/tmp/base_config.yaml") as fh: data = yaml.safe_load(fh) @@ -220,36 +188,23 @@ jobs: sys.stderr.write("maintainers entry is not a non-empty string\n") sys.exit(2) names.append(m.strip().lower()) - # Normalized, de-duplicated, sorted for stable comparison. for n in sorted(set(names)): print(n) PY PARSE_RC=${PARSE_RC:-0} - if [ "$PARSE_RC" -ne 0 ]; then - not_eligible "base config.yaml maintainers could not be parsed" - fi + [ "$PARSE_RC" -eq 0 ] || not_eligible "base config.yaml maintainers could not be parsed" - # 5. SECURITY: if config.yaml is among the changed files, ensure the - # PR does NOT modify the maintainers field. Changing who holds - # admin power must go through human review. Compare the normalized - # maintainers list of base vs head of config.yaml. + # 4. SECURITY: if config.yaml is changed, the maintainers field must be + # unchanged (changing who holds power needs human review). Compare + # the normalized base vs head maintainers list. if printf '%s\n' "${CHANGED[@]}" | grep -qxF "$CONFIG_PATH"; then - echo "config.yaml is modified; verifying maintainers field is unchanged" - - # Fetch the HEAD version of config.yaml via the API (raw contents at - # the PR head sha, from $HEAD_SHA in env). We parse it ONLY to compare - # the maintainers field; we never execute it. A failure to fetch/parse - # the head config => not eligible (conservative default). This also - # catches a config.yaml that the PR deleted or renamed away (the fetch - # 404s), so admin-list removal cannot ride through. - if ! gh api \ - -H "Accept: application/vnd.github.raw+json" \ + echo "config.yaml changed; verifying the maintainers field is unchanged" + if ! gh api -H "Accept: application/vnd.github.raw+json" \ "/repos/$REPO/contents/$CONFIG_PATH?ref=$HEAD_SHA" \ > /tmp/head_config.yaml 2>/dev/null; then - not_eligible "could not read head config.yaml for maintainers diff" + not_eligible "could not read head config.yaml (delete/rename needs human review)" fi - - python - "$COMMUNITY" <<'PY' > /tmp/head_maintainers.txt || HEAD_RC=$? + python - <<'PY' > /tmp/head_maintainers.txt || HEAD_RC=$? import sys, yaml try: with open("/tmp/head_config.yaml") as fh: @@ -274,37 +229,30 @@ jobs: print(n) PY HEAD_RC=${HEAD_RC:-0} - if [ "$HEAD_RC" -ne 0 ]; then - not_eligible "head config.yaml maintainers could not be parsed" - fi - + [ "$HEAD_RC" -eq 0 ] || not_eligible "head config.yaml maintainers could not be parsed" if ! diff -q /tmp/base_maintainers.txt /tmp/head_maintainers.txt >/dev/null; then not_eligible "PR modifies the maintainers field (needs human review)" fi echo "maintainers field unchanged; OK to proceed" fi - # 6. Verify the PR author is in the BASE-branch maintainers list. - # GitHub usernames are case-insensitive, so compare lowercased. - # AUTHOR_LOGIN comes from env (never interpolated into the script). - AUTHOR="$(printf '%s' "$AUTHOR_LOGIN" | tr '[:upper:]' '[:lower:]')" - echo "PR author (normalized): $AUTHOR" - if ! grep -qxF "$AUTHOR" /tmp/base_maintainers.txt; then - not_eligible "author '$AUTHOR' is not a maintainer of '$COMMUNITY'" + # 5. The COMMENTER must be a maintainer of this community (base list). + # GitHub usernames are case-insensitive; compare lowercased. + COMMENTER="$(printf '%s' "$COMMENTER_LOGIN" | tr '[:upper:]' '[:lower:]')" + echo "Commenter (normalized): $COMMENTER" + if ! grep -qxF "$COMMENTER" /tmp/base_maintainers.txt; then + not_eligible "commenter '$COMMENTER' is not a maintainer of '$COMMUNITY'" fi - # All checks passed. - echo "::notice::PR #$PR_NUMBER is eligible for auto-merge (community=$COMMUNITY, author=$AUTHOR)" + echo "::notice::Merging PR #$PR_NUMBER ($COMMUNITY) into develop on @$COMMENTER command; head $HEAD_SHA" { echo "eligible=true" echo "community=$COMMUNITY" - echo "reason=author is a maintainer; changes scoped to $COMMUNITY" + echo "head_sha=$HEAD_SHA" + echo "commenter=$COMMENTER" } >> "$GITHUB_OUTPUT" - # ---------------------------------------------------------------------- - # Mint the GitHub App installation token. Only reached when eligible. - # Short-lived; scoped to this repo by the App installation. - # ---------------------------------------------------------------------- + # Mint the GitHub App installation token. Only reached when authorized. - name: Mint GitHub App token if: steps.eval.outputs.eligible == 'true' id: app_token @@ -313,83 +261,71 @@ jobs: app-id: ${{ secrets.COMMUNITY_MERGE_APP_ID }} private-key: ${{ secrets.COMMUNITY_MERGE_APP_PRIVATE_KEY }} - # Approve the PR using the App token. A clear, attributable review body. + # Record an approval (audit trail) attributing the merge to the commenter. - name: Approve PR if: steps.eval.outputs.eligible == 'true' env: GH_TOKEN: ${{ steps.app_token.outputs.token }} REPO: ${{ github.repository }} - PR_NUMBER: ${{ github.event.pull_request.number }} + PR_NUMBER: ${{ github.event.issue.number }} COMMUNITY: ${{ steps.eval.outputs.community }} + COMMENTER: ${{ steps.eval.outputs.commenter }} run: | set -euo pipefail - gh api \ - --method POST \ - -H "Accept: application/vnd.github+json" \ + gh api --method POST -H "Accept: application/vnd.github+json" \ "/repos/$REPO/pulls/$PR_NUMBER/reviews" \ -f event=APPROVE \ - -f body="Auto-approved by community-admin auto-merge: changes are scoped to the **$COMMUNITY** community directory and the author is a listed maintainer. Required status checks still gate the merge." + -f body="Merge approved on @$COMMENTER's LGTM/merge command. Changes are scoped to the **$COMMUNITY** community directory; required status checks still gate the merge." - # Enable auto-merge with the App token. --auto means the merge happens only - # once all required checks (ruff + tests) pass, so we never bypass CI. The - # merge method is chosen by target branch to satisfy the branch rulesets: - # develop allows squash (feature-branch convention); main is merge-commit - # only (the develop->main release convention). Using --squash on a main PR - # would be rejected by the protect-main ruleset. - - name: Enable auto-merge + # Enable squash auto-merge into develop, PINNED to the head the command was + # made on. --auto waits for required checks; --match-head-commit means a + # push after the command cancels this and requires a fresh "LGTM ... merge". + - name: Merge on command (pinned to the commented head) if: steps.eval.outputs.eligible == 'true' env: GH_TOKEN: ${{ steps.app_token.outputs.token }} REPO: ${{ github.repository }} - PR_NUMBER: ${{ github.event.pull_request.number }} - BASE_REF: ${{ github.event.pull_request.base.ref }} + PR_NUMBER: ${{ github.event.issue.number }} + HEAD_SHA: ${{ steps.eval.outputs.head_sha }} run: | set -euo pipefail - if [ "$BASE_REF" = "main" ]; then - METHOD="--merge" - else - METHOD="--squash" - fi - echo "Merging PR #$PR_NUMBER into $BASE_REF with $METHOD" - gh pr merge "$PR_NUMBER" --repo "$REPO" --auto "$METHOD" + echo "Squash-merging PR #$PR_NUMBER into develop, pinned to $HEAD_SHA" + gh pr merge "$PR_NUMBER" --repo "$REPO" --auto --squash --match-head-commit "$HEAD_SHA" - # Post (find-or-update) a confirmation comment so we do not spam a new - # comment on every synchronize event. Uses the App token's identity. - - name: Comment confirmation + # Find-or-update a single confirmation comment (no spam on repeat commands). + - name: Comment result if: steps.eval.outputs.eligible == 'true' continue-on-error: true uses: actions/github-script@v7 + env: + COMMUNITY: ${{ steps.eval.outputs.community }} + COMMENTER: ${{ steps.eval.outputs.commenter }} with: github-token: ${{ steps.app_token.outputs.token }} script: | const community = process.env.COMMUNITY; - const marker = ''; + const commenter = process.env.COMMENTER; + const marker = ''; const body = `${marker}\n` + - `Auto-merge enabled for the **${community}** community.\n\n` + - `This PR only touches \`src/assistants/${community}/\` and was opened by a ` + - `listed maintainer, so it has been approved and queued for auto-merge. ` + - `It will merge automatically once all required checks (ruff + tests) pass.`; - + `Queued for merge on @${commenter}'s **LGTM / merge** command.\n\n` + + `This PR only touches \`src/assistants/${community}/\`, so it has been ` + + `approved and will merge automatically once all required checks pass. ` + + `The merge is pinned to the current commit; pushing new commits cancels ` + + `it and needs a fresh \`LGTM ... merge\` comment.`; const { data: comments } = await github.rest.issues.listComments({ owner: context.repo.owner, repo: context.repo.repo, - issue_number: context.payload.pull_request.number, + issue_number: context.payload.issue.number, }); const existing = comments.find(c => c.body && c.body.includes(marker)); if (existing) { await github.rest.issues.updateComment({ - owner: context.repo.owner, - repo: context.repo.repo, - comment_id: existing.id, - body, + owner: context.repo.owner, repo: context.repo.repo, + comment_id: existing.id, body, }); } else { await github.rest.issues.createComment({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.payload.pull_request.number, - body, + owner: context.repo.owner, repo: context.repo.repo, + issue_number: context.payload.issue.number, body, }); } - env: - COMMUNITY: ${{ steps.eval.outputs.community }} diff --git a/CLAUDE.md b/CLAUDE.md index f8be9aa..3ec24c1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -162,7 +162,7 @@ src/ - **.context/yaml_registry.md** - YAML-based community config (internal notes) - **.context/community_onboarding_review.md** - Onboarding gap analysis - **.context/local-testing-guide.md** - Quick local testing reference -- **.context/community-admin-merge.md** - Maintainer auto-merge for community-scoped PRs (setup + trust model) +- **.context/community-admin-merge.md** - Community maintainer "LGTM/merge" comment command to merge community-scoped PRs into develop (setup + trust model) - **Existing configs to reference**: `src/assistants/hed/config.yaml`, `src/assistants/eeglab/config.yaml` ### Tool System From 268cb62ef5b6550ac4459c500e967e0beb7f5f41 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 5 Jun 2026 06:16:46 +0000 Subject: [PATCH 21/39] Bump version to 0.8.4.dev10 --- src/version.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/version.py b/src/version.py index 0741f2c..bac355e 100644 --- a/src/version.py +++ b/src/version.py @@ -1,7 +1,7 @@ """Version information for OSA.""" -__version__ = "0.8.4.dev9" -__version_info__ = (0, 8, 4, "dev9") +__version__ = "0.8.4.dev10" +__version_info__ = (0, 8, 4, "dev10") def get_version() -> str: From 9e2adc94aff3806b14355f1c5699a64e3e395a3f Mon Sep 17 00:00:00 2001 From: "Seyed (Yahya) Shirazi" Date: Thu, 4 Jun 2026 23:24:56 -0700 Subject: [PATCH 22/39] test(hed): verify LGTM/merge comment command (#301) --- src/assistants/hed/config.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/src/assistants/hed/config.yaml b/src/assistants/hed/config.yaml index c7d6562..718b6f0 100644 --- a/src/assistants/hed/config.yaml +++ b/src/assistants/hed/config.yaml @@ -466,3 +466,4 @@ extensions: # Community maintainers listed above may open scoped PRs (touching only # this directory) that auto-approve and auto-merge via the community-admin # merge workflow. +# (verifying the LGTM/merge comment command) From 8aa4b55f8ce0203fa75c81e6683d51f10f975b6c Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 5 Jun 2026 06:25:09 +0000 Subject: [PATCH 23/39] Bump version to 0.8.4.dev11 --- src/version.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/version.py b/src/version.py index bac355e..9b4e956 100644 --- a/src/version.py +++ b/src/version.py @@ -1,7 +1,7 @@ """Version information for OSA.""" -__version__ = "0.8.4.dev10" -__version_info__ = (0, 8, 4, "dev10") +__version__ = "0.8.4.dev11" +__version_info__ = (0, 8, 4, "dev11") def get_version() -> str: From ffccf1fa2105208feb6be00b2c4e1f4422f5bf85 Mon Sep 17 00:00:00 2001 From: "Seyed (Yahya) Shirazi" Date: Thu, 4 Jun 2026 23:36:01 -0700 Subject: [PATCH 24/39] fix(ci): ignore bot comments in the community merge command (#303) The App's confirmation comment contains 'LGTM / merge' and re-triggered the issue_comment workflow (which then no-op'd: bot is not a maintainer). Skip comments where comment.user.type == 'Bot' so the App's own comment no longer spawns an extra run. Closes #302 --- .github/workflows/community-admin-pr-merge.yml | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/.github/workflows/community-admin-pr-merge.yml b/.github/workflows/community-admin-pr-merge.yml index 8e96c31..48b5a58 100644 --- a/.github/workflows/community-admin-pr-merge.yml +++ b/.github/workflows/community-admin-pr-merge.yml @@ -50,8 +50,12 @@ concurrency: jobs: merge-on-command: - # Only comments on pull requests (issue_comment also fires on plain issues). - if: github.event.issue.pull_request != null + # Only human comments on pull requests. issue_comment also fires on plain + # issues (filtered out), and on the App's own confirmation comment (which + # mentions "LGTM / merge") -- skipping Bot authors stops that self-trigger. + if: >- + github.event.issue.pull_request != null && + github.event.comment.user.type != 'Bot' runs-on: ubuntu-latest steps: # Cheap first gate: does the comment carry the "LGTM ... merge" command? From 72199ba1d08cbb245eca0f41555e521860181135 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 5 Jun 2026 06:36:14 +0000 Subject: [PATCH 25/39] Bump version to 0.8.4.dev12 --- src/version.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/version.py b/src/version.py index 9b4e956..921c81f 100644 --- a/src/version.py +++ b/src/version.py @@ -1,7 +1,7 @@ """Version information for OSA.""" -__version__ = "0.8.4.dev11" -__version_info__ = (0, 8, 4, "dev11") +__version__ = "0.8.4.dev12" +__version_info__ = (0, 8, 4, "dev12") def get_version() -> str: From 073d2608a4772fe79b5d974eb84d4c080be20a31 Mon Sep 17 00:00:00 2001 From: "Seyed (Yahya) Shirazi" Date: Fri, 5 Jun 2026 00:45:30 -0700 Subject: [PATCH 26/39] fix(search): match multi-word FTS queries (#306) * fix(search): match multi-word FTS queries Replace blanket phrase-wrapping in _sanitize_fts5_query with tokenize -> drop stopwords/operators -> quote each term -> OR. Multi-word queries (every query the agent sends for papers/discussions/FAQ/docstrings) were exact-phrase matched and returned nothing despite populated databases. Still injection-safe (each term individually quoted); results ordered by existing BM25 rank. Affects all 6 knowledge-search call sites. Closes #305 * test(search): harden operator test; keep list/use terms Address PR review: - strengthen test_sanitize_fts5_operators to assert every term is individually quoted (no bare operator can reach MATCH) - drop 'list' and 'use' from stopwords (meaningful EEGLAB/MATLAB nouns) so multi-word queries like 'list channels' don't silently lose a term * test(bep): use non-matching query for no-results case BEP keyword search is OR-based and rank-ordered after the FTS fix, so the old phrase-only no-match query ('...data type...') now matches BEPs that mention 'data'. Use a genuinely non-matching query to keep the no-results path covered. --- src/knowledge/search.py | 108 +++++++++++++++++++++++++--- tests/test_knowledge/test_search.py | 74 ++++++++++++++++--- tests/test_tools/test_bep_tool.py | 5 +- 3 files changed, 165 insertions(+), 22 deletions(-) diff --git a/src/knowledge/search.py b/src/knowledge/search.py index 1130644..61563f2 100644 --- a/src/knowledge/search.py +++ b/src/knowledge/search.py @@ -98,26 +98,112 @@ def _titles_are_similar( return similarity >= threshold +# Common English words that add noise to keyword search without improving +# relevance. Deliberately short and domain-agnostic so we never strip a +# meaningful term (acronyms, function names, identifiers are all kept). +_FTS_STOPWORDS = frozenset( + { + "a", + "an", + "and", + "any", + "are", + "as", + "about", + "at", + "be", + "but", + "by", + "can", + "did", + "do", + "does", + "for", + "from", + "give", + "has", + "have", + "how", + "i", + "in", + "into", + "is", + "it", + "its", + "me", + "my", + "no", + "not", + "of", + "on", + "or", + "paper", + "papers", + "please", + "research", + "search", + "show", + "some", + "tell", + "that", + "the", + "their", + "them", + "there", + "these", + "this", + "to", + "using", + "want", + "was", + "we", + "what", + "when", + "where", + "which", + "who", + "why", + "will", + "with", + "would", + "you", + "your", + } +) + + def _sanitize_fts5_query(query: str) -> str: - """Sanitize user input for safe FTS5 queries. + """Build a safe, forgiving FTS5 MATCH expression from raw user input. - IMPORTANT: This function wraps ALL input in quotes, converting queries to - exact phrase searches. This prevents FTS5 operator injection but also - disables legitimate FTS5 features (AND/OR/NOT, wildcards, NEAR, etc.). + Splits the query into individual terms, drops noise words and any FTS5 + operator characters, quotes each remaining term (so reserved words like + AND/OR/NEAR and punctuation cannot inject operators), and ORs them + together. Callers order results by BM25 ``rank``, so documents matching + the most (and rarest) terms surface first. - For a production system with advanced search needs, consider implementing - proper query parsing instead of blanket phrase conversion. + This replaces the previous behaviour of wrapping the whole query in quotes, + which forced an exact consecutive-phrase match and caused multi-word + natural-language queries to return nothing. Args: query: Raw user input Returns: - Sanitized query safe for FTS5 MATCH (as a phrase search) + A MATCH expression safe from FTS5 injection. Falls back to a quoted + phrase of the raw input when no meaningful terms remain (e.g. a query + made entirely of stopwords or symbols), preserving safe behaviour + instead of producing an empty MATCH. """ - # Escape internal double quotes by doubling them - escaped = query.replace('"', '""') - # Wrap in quotes to treat entire input as phrase search - return f'"{escaped}"' + # Tokens: words, numbers, and identifier-style names (pop_loadset, clean_rawdata). + # The regex strips all FTS5 operator characters (quotes, *, :, (), -, etc.). + tokens = re.findall(r"[A-Za-z0-9_]+", query.lower()) + terms = [t for t in tokens if t not in _FTS_STOPWORDS] + if not terms: + # Nothing meaningful left: fall back to a safe phrase match of raw input. + escaped = query.replace('"', '""') + return f'"{escaped}"' + # Quote each term individually to neutralize operators, then OR them. + return " OR ".join(f'"{t}"' for t in terms) @dataclass diff --git a/tests/test_knowledge/test_search.py b/tests/test_knowledge/test_search.py index 2460bbb..396c017 100644 --- a/tests/test_knowledge/test_search.py +++ b/tests/test_knowledge/test_search.py @@ -3,6 +3,7 @@ These tests use a temporary database populated with test data. """ +import re from pathlib import Path from unittest.mock import patch @@ -171,6 +172,28 @@ def test_filter_by_source(self, populated_db: Path): assert all(r.source == "openalex" for r in openalex_results) assert all(r.source == "semanticscholar" for r in s2_results) + def test_multiword_query_matches_non_adjacent_terms(self, populated_db: Path): + """Regression (issue #305): natural-language queries must match papers + even when the terms are not adjacent in the document. + + Previously the whole query was phrase-wrapped, so multi-word questions + returned zero papers despite a populated database. "annotation" and + "neuroimaging" both appear in a paper but never consecutively in this + order, so the old phrase match returned nothing. + """ + with patch("src.knowledge.db.get_db_path", return_value=populated_db): + results = search_papers("annotation HED neuroimaging") + + assert len(results) >= 1 + assert any("HED Annotation" in r.title for r in results) + + def test_question_phrasing_finds_papers(self, populated_db: Path): + """A full question (with stopwords) still surfaces relevant papers.""" + with patch("src.knowledge.db.get_db_path", return_value=populated_db): + results = search_papers("what papers are about HED annotation?") + + assert len(results) >= 1 + class TestSearchAll: """Tests for combined search.""" @@ -317,18 +340,43 @@ def test_number_lookup_deduplicates(self, populated_db: Path): class TestFTS5Sanitization: """Tests for FTS5 query sanitization to prevent injection.""" - def test_sanitize_basic_query(self): - """Test that basic queries are wrapped in quotes.""" + def test_sanitize_splits_into_or_terms(self): + """Basic queries become an OR of individually quoted terms.""" result = _sanitize_fts5_query("validation error") - assert result == '"validation error"' + assert result == '"validation" OR "error"' + + def test_sanitize_drops_stopwords(self): + """Noise words are removed; meaningful terms (incl. acronyms) kept.""" + result = _sanitize_fts5_query("what papers are about ICA") + assert result == '"ica"' + + def test_sanitize_keeps_identifier_tokens(self): + """Function/identifier names stay intact (underscores preserved).""" + result = _sanitize_fts5_query("pop_runica parameters") + assert result == '"pop_runica" OR "parameters"' + + def test_sanitize_keeps_command_noun_terms(self): + """Words that double as content/command nouns are not stopwords. - def test_sanitize_escapes_quotes(self): - """Test that double quotes in user input are escaped.""" + 'list' and 'use' carry meaning in EEGLAB/MATLAB queries (e.g. channel + lists), so they must survive instead of being silently dropped. + """ + assert _sanitize_fts5_query("list channels") == '"list" OR "channels"' + assert _sanitize_fts5_query("use function") == '"use" OR "function"' + + def test_sanitize_strips_quotes_no_injection(self): + """Double quotes in input are stripped by tokenization, not escaped in.""" result = _sanitize_fts5_query('say "hello" world') - assert result == '"say ""hello"" world"' + assert result == '"say" OR "hello" OR "world"' + assert '""' not in result + + def test_sanitize_stopword_only_query_falls_back(self): + """A query of only stopwords falls back to a safe quoted phrase.""" + result = _sanitize_fts5_query("what are the") + assert result == '"what are the"' def test_sanitize_fts5_operators(self): - """Test that FTS5 operators are treated as literal text.""" + """FTS5 operators are quoted as literal terms, never executed.""" # These would be dangerous without sanitization dangerous_queries = [ "test AND DROP TABLE", @@ -339,9 +387,15 @@ def test_sanitize_fts5_operators(self): ] for query in dangerous_queries: result = _sanitize_fts5_query(query) - # Should be wrapped in quotes, treating operators as literals - assert result.startswith('"') - assert result.endswith('"') + # Every OR-separated term must be individually double-quoted, so no + # bare FTS5 operator (AND/OR/NOT/NEAR/wildcard) can reach MATCH. + for term in result.split(" OR "): + assert term.startswith('"') and term.endswith('"'), ( + f"unquoted term {term!r} in result for {query!r}: {result!r}" + ) + # Removing every quoted term must leave only ' OR ' connectors. + remainder = re.sub(r'"[^"]*"', "", result).replace(" OR ", "").strip() + assert remainder == "", f"stray operator text in result for {query!r}: {result!r}" def test_search_handles_special_characters(self, populated_db: Path): """Test that search doesn't crash with special FTS5 characters.""" diff --git a/tests/test_tools/test_bep_tool.py b/tests/test_tools/test_bep_tool.py index 411cec8..4d7de3f 100644 --- a/tests/test_tools/test_bep_tool.py +++ b/tests/test_tools/test_bep_tool.py @@ -122,7 +122,10 @@ def test_lookup_no_results(self, bep_db: Path): patch("src.assistants.bids.tools.get_db_path", return_value=bep_db), patch("src.knowledge.db.get_db_path", return_value=bep_db), ): - result = lookup_bep.invoke({"query": "nonexistent data type xyz"}) + # Keyword search is OR-based and rank-ordered, so the query must + # contain no real terms present in any BEP to return zero results + # (e.g. "data" would match many BEP descriptions). + result = lookup_bep.invoke({"query": "qwertyuiop zxcvbnmlkj"}) assert "No BEPs found" in result From dfd5d68f02987ab0ec8c0b8b18d024810e5333af Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 5 Jun 2026 07:45:42 +0000 Subject: [PATCH 27/39] Bump version to 0.8.4.dev13 --- src/version.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/version.py b/src/version.py index 921c81f..7ea87eb 100644 --- a/src/version.py +++ b/src/version.py @@ -1,7 +1,7 @@ """Version information for OSA.""" -__version__ = "0.8.4.dev12" -__version_info__ = (0, 8, 4, "dev12") +__version__ = "0.8.4.dev13" +__version_info__ = (0, 8, 4, "dev13") def get_version() -> str: From b1814f9ada037938425d002fd7fc74f5fdb1c5bb Mon Sep 17 00:00:00 2001 From: "Seyed (Yahya) Shirazi" Date: Fri, 5 Jun 2026 00:49:07 -0700 Subject: [PATCH 28/39] refactor(papers): use opencite for paper sync + declare dependency (#309) * refactor(papers): use opencite for paper sync Replace the hand-rolled OpenAlex/Semantic Scholar/PubMed fetchers (and inverted-index reconstruction) with the opencite multi-source client, which aggregates and deduplicates across sources. Public sync function signatures are unchanged, so the CLI and scheduler call them as before; only the fetch layer is swapped. opencite Paper objects map to stable (source, external_id) pairs compatible with existing rows. Declares opencite>=0.5.2 as a server dependency, which also attributes OSA to neuromechanist/opencite in GitHub's dependency graph. Config is constructed directly (not Config.from_env) so sync never depends on ambient .env files in the working directory. Closes #307 * refactor(papers): share one opencite client per batch; guard async bridge Address PR review: - sync_all_papers/sync_citing_papers now open a single SearchOrchestrator/ CitationExplorer for the whole batch instead of one per query/DOI (each open spins up 11 HTTP clients). Per-item errors are isolated so one bad query/DOI no longer aborts the batch. - Add _run() helper that uses asyncio.run normally but offloads to a worker thread if a loop is already running, so the public sync functions are safe to call from any context (CLI, scheduler thread, or future async caller). - Test _run in both the no-loop and running-loop paths. --- pyproject.toml | 1 + src/knowledge/papers_sync.py | 697 ++++++++++------------- tests/test_knowledge/test_papers_sync.py | 254 +++++---- uv.lock | 17 + 4 files changed, 464 insertions(+), 505 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index e54707c..83f9a74 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -66,6 +66,7 @@ server = [ "markdownify>=1.1.0", # Scheduling "apscheduler>=3.10.0,<4.0.0", + "opencite>=0.5.2", ] observability = [ diff --git a/src/knowledge/papers_sync.py b/src/knowledge/papers_sync.py index 95eea0d..640ad4c 100644 --- a/src/knowledge/papers_sync.py +++ b/src/knowledge/papers_sync.py @@ -1,258 +1,289 @@ -"""Paper sync from OpenALEX, Semantic Scholar, and PubMed Central. +"""Paper sync backed by opencite. -Syncs papers for community-configured search queries. -Only stores title, abstract snippet, URL, and publication date. +Fetches papers through the `opencite` multi-source search/citation client and +writes them into the local knowledge database. opencite aggregates and +deduplicates across OpenAlex, Semantic Scholar, PubMed (and more), replacing +the previous hand-rolled per-source fetchers and inverted-index handling. -Rate limits: -- OpenALEX: No key required, generous limits -- Semantic Scholar: ~100 requests/5 min (free), higher with API key -- PubMed: ~3 requests/sec without key, 10/sec with key +Public sync functions keep their original signatures so the CLI +(`src/cli/sync.py`) and the scheduler (`src/api/scheduler.py`) call them +unchanged; only the fetch layer is swapped. + +See: https://github.com/neuromechanist/opencite """ +import asyncio import logging -import time -import xml.etree.ElementTree as ET -from typing import Any +from collections.abc import Iterable +from concurrent.futures import ThreadPoolExecutor -import httpx -import pyalex -from pyalex import Works +from opencite import Config, Paper +from opencite.citations import CitationExplorer +from opencite.search import SearchOrchestrator from src.knowledge.db import get_connection, update_sync_metadata, upsert_paper logger = logging.getLogger(__name__) -# Rate limiting settings -SEMANTIC_SCHOLAR_DELAY = 3.0 # seconds between requests (to stay under 100/5min) -PUBMED_DELAY = 0.4 # seconds between requests (to stay under 3/sec) +# Scholarly sources synced by default. opencite also supports arxiv, biorxiv, +# medrxiv, osf, zenodo, figshare, crossref and core; those broader sources are +# reserved for the opt-in live-search feature (issue #308) so batch sync stays +# focused on peer-reviewed literature and matches prior coverage. +DEFAULT_SOURCES: tuple[str, ...] = ("openalex", "s2", "pubmed") + +# opencite source name -> OSA `papers.source` label. Kept stable so dedup and +# the existing rows in the database (openalex / semanticscholar / pubmed) line +# up with newly synced papers. +_OSA_SOURCE_BY_OPENCITE: dict[str, str] = { + "openalex": "openalex", + "s2": "semanticscholar", + "pubmed": "pubmed", +} +# OSA source label -> opencite source name (used to restrict per-source syncs). +_OPENCITE_SOURCE_BY_OSA: dict[str, str] = {v: k for k, v in _OSA_SOURCE_BY_OPENCITE.items()} + +# OpenAlex credentials set via configure_openalex(); merged into the per-sync +# Config as a fallback when explicit call arguments are not supplied. This +# preserves the CLI's "configure once, sync many" pattern. +_OPENALEX_API_KEY: str | None = None +_OPENALEX_EMAIL: str | None = None def configure_openalex(api_key: str | None = None, email: str | None = None) -> None: - """Configure pyalex with API key or email for polite pool access. + """Store OpenAlex credentials for subsequent opencite-backed syncs. + + OpenAlex works anonymously; an API key grants premium limits and a contact + email enables the faster polite pool. Values are merged into the opencite + Config built for each sync (explicit per-call arguments still win). Args: - api_key: OpenAlex API key for premium access (~2M requests). - email: Email for polite pool access (faster than anonymous). + api_key: OpenAlex API key for premium access. + email: Contact email for OpenAlex polite pool access. """ - # Treat empty strings as None - api_key = api_key.strip() if api_key else None - email = email.strip() if email else None + global _OPENALEX_API_KEY, _OPENALEX_EMAIL + _OPENALEX_API_KEY = api_key.strip() if api_key and api_key.strip() else None + _OPENALEX_EMAIL = email.strip() if email and email.strip() else None - if api_key: - pyalex.config.api_key = api_key + if _OPENALEX_API_KEY: logger.info("OpenAlex configured with API key") - elif email: - pyalex.config.email = email - logger.info("OpenAlex configured with email: %s (polite pool)", email) + elif _OPENALEX_EMAIL: + logger.info("OpenAlex configured with email: %s (polite pool)", _OPENALEX_EMAIL) else: logger.debug("OpenAlex using anonymous access (lower rate limits)") -def _reconstruct_abstract(inverted_index: dict[str, list[int]] | None) -> str: - """Reconstruct abstract from OpenALEX inverted index format. - - OpenALEX stores abstracts as inverted indexes: {"word": [positions]} - This function reconstructs the original text. - - Args: - inverted_index: Dict mapping words to their positions - - Returns: - Reconstructed abstract text - """ - if not inverted_index: - return "" - - # Find max position to size the array - max_pos = 0 - for positions in inverted_index.values(): - if positions: - max_pos = max(max_pos, max(positions)) - - # Build word array - words = [""] * (max_pos + 1) - for word, positions in inverted_index.items(): - for pos in positions: - words[pos] = word - - return " ".join(words) - - -def _get_paper_url(doi: str | None, fallback_id: str) -> str: - """Get paper URL, preferring DOI when available. - - Args: - doi: The DOI (may be full URL or bare DOI) - fallback_id: Fallback URL/ID if no DOI - - Returns: - URL string +def _build_config( + *, + openalex_api_key: str | None = None, + openalex_email: str | None = None, + semantic_scholar_api_key: str | None = None, + pubmed_api_key: str | None = None, +) -> Config: + """Build an opencite Config from explicit args and configure_openalex(). + + Credentials come from OSA settings (passed explicitly) with a fallback to + values set via configure_openalex(). We construct Config directly rather + than Config.from_env() so paper sync never depends on ambient ``.env`` + files in the working directory, which are environment-specific and have + tripped opencite's dotenv loader. """ - if not doi: - return fallback_id - return doi if doi.startswith("http") else f"https://doi.org/{doi}" - - -def _get_openalex_external_id(openalex_id: str) -> str: - """Extract external ID from OpenALEX URL. - - Args: - openalex_id: Full OpenALEX URL or bare ID - - Returns: - Bare external ID (e.g., "W12345") + return Config( + openalex_api_key=openalex_api_key or _OPENALEX_API_KEY or "", + contact_email=openalex_email or _OPENALEX_EMAIL or "", + semantic_scholar_api_key=semantic_scholar_api_key or "", + pubmed_api_key=pubmed_api_key or "", + ) + + +def _native_id(paper: Paper, osa_source: str) -> str: + """Return the identifier matching a specific OSA source label, or ''.""" + ids = paper.ids + if osa_source == "openalex": + return ids.openalex_id.removeprefix("https://openalex.org/") if ids.openalex_id else "" + if osa_source == "semanticscholar": + return ids.s2_id or "" + if osa_source == "pubmed": + return ids.pmid or "" + return "" + + +def _paper_source_and_id(paper: Paper) -> tuple[str | None, str | None]: + """Pick a stable (source, external_id) for the papers table. + + Prefers identifiers in the order OpenAlex > Semantic Scholar > PubMed > DOI + > arXiv so a paper maps to the same row across syncs and aligns with rows + already stored from the previous per-source fetchers. Returns (None, None) + when no usable identifier is present (such papers are skipped). """ - return openalex_id.removeprefix("https://openalex.org/") - - -def sync_openalex_papers(query: str, max_results: int = 100, project: str = "hed") -> int: - """Sync papers from OpenALEX matching query. + ids = paper.ids + openalex = ids.openalex_id.removeprefix("https://openalex.org/") if ids.openalex_id else "" + if openalex: + return "openalex", openalex + if ids.s2_id: + return "semanticscholar", ids.s2_id + if ids.pmid: + return "pubmed", ids.pmid + if ids.doi: + return "doi", ids.doi.lower() + if ids.arxiv_id: + return "arxiv", ids.arxiv_id + return None, None + + +def _paper_url(paper: Paper) -> str: + """Best link for a paper, preferring a stable DOI landing page.""" + if paper.doi: + return f"https://doi.org/{paper.doi}" + if paper.url: + return paper.url + if paper.best_pdf_url: + return paper.best_pdf_url + return "" + + +def _store_papers( + papers: Iterable[Paper], + project: str, + *, + force_source: str | None = None, +) -> dict[str, int]: + """Upsert opencite papers into the knowledge DB, returning counts by source. Args: - query: Search query - max_results: Maximum number of papers to sync - project: Assistant/project name for database isolation. Defaults to 'hed'. - - Returns: - Number of papers synced + papers: opencite Paper objects to store. + project: Community/project ID for database isolation. + force_source: When set (a single-source sync), record this OSA source + label using its native identifier; falls back to the priority + mapping if that identifier is missing. """ - logger.info("Syncing OpenALEX papers for query: %s", query) - - try: - # Build query and fetch results - # pyalex returns a lazy query object, need to call .get() to fetch results - works_query = ( - Works() - .search(query) - .select( - [ - "id", - "title", - "abstract_inverted_index", - "publication_date", - "doi", - "primary_location", - ] - ) - ) - # Fetch up to max_results using pagination - works = list(works_query.get(per_page=min(max_results, 200))) - except Exception as e: - logger.warning("OpenALEX error for '%s': %s", query, e) - return 0 - - count = 0 + counts: dict[str, int] = {} with get_connection(project) as conn: - for work in works: - if count >= max_results: - break - - # Skip if no title - title = work.get("title") - if not title: + for paper in papers: + if not paper.title: continue - abstract = _reconstruct_abstract(work.get("abstract_inverted_index")) - url = _get_paper_url(work.get("doi"), work.get("id", "")) - external_id = _get_openalex_external_id(work.get("id", "")) + if force_source: + external_id = _native_id(paper, force_source) + source: str | None = force_source if external_id else None + if not source: + source, external_id = _paper_source_and_id(paper) + else: + source, external_id = _paper_source_and_id(paper) + + if not source or not external_id: + continue upsert_paper( conn, - source="openalex", + source=source, external_id=external_id, - title=title, - first_message=abstract, - url=url, - created_at=work.get("publication_date"), + title=paper.title, + first_message=paper.abstract or None, + url=_paper_url(paper), + created_at=paper.publication_date or (str(paper.year) if paper.year else None), ) - count += 1 - + counts[source] = counts.get(source, 0) + 1 conn.commit() + return counts - logger.info("Synced %d papers from OpenALEX for '%s'", count, query) - update_sync_metadata("papers", f"openalex:{query}", count, project) - return count +def _run(coro): + """Execute an async coroutine from synchronous code. -def sync_semanticscholar_papers( + OSA's sync callers (CLI command, scheduler thread) have no running event + loop, so asyncio.run is used directly. If a loop is already running in the + calling thread, the coroutine runs in a dedicated worker thread so these + public sync functions stay safe to call from any context. + """ + try: + asyncio.get_running_loop() + except RuntimeError: + return asyncio.run(coro) + with ThreadPoolExecutor(max_workers=1) as pool: + return pool.submit(asyncio.run, coro).result() + + +async def _search_queries( + config: Config, + queries: list[str], + max_results: int, + sources: tuple[str, ...] | None, +) -> list[tuple[str, list[Paper]]]: + """Search every query through one shared opencite orchestrator. + + A single orchestrator (and its HTTP client pool) is opened for the whole + batch. A failure for an individual query is logged and yields an empty + result for that query rather than aborting the batch. + """ + out: list[tuple[str, list[Paper]]] = [] + async with SearchOrchestrator(config) as searcher: + for query in queries: + try: + result = await searcher.search(query, max_results=max_results, sources=sources) + out.append((query, result.papers)) + except Exception as e: + logger.warning("opencite search error for '%s': %s", query, e) + out.append((query, [])) + return out + + +async def _citing_for_dois( + config: Config, + dois: list[str], + max_results: int, +) -> list[tuple[str, list[Paper]]]: + """Fetch citing papers for every DOI through one shared CitationExplorer.""" + out: list[tuple[str, list[Paper]]] = [] + async with CitationExplorer(config) as explorer: + for doi in dois: + try: + result = await explorer.citing_papers(doi, max_results=max_results) + out.append((doi, result.papers)) + except Exception as e: + logger.warning("opencite citation error for DOI %s: %s", doi, e) + out.append((doi, [])) + return out + + +def _sync_single_source( query: str, - max_results: int = 100, - api_key: str | None = None, - project: str = "hed", + max_results: int, + project: str, + osa_source: str, + config: Config, ) -> int: - """Sync papers from Semantic Scholar matching query. - - Args: - query: Search query - max_results: Maximum number of papers to sync - api_key: Optional API key for higher rate limits - project: Assistant/project name for database isolation. Defaults to 'hed'. - - Returns: - Number of papers synced - """ - logger.info("Syncing Semantic Scholar papers for query: %s", query) - - url = "https://api.semanticscholar.org/graph/v1/paper/search" - params: dict[str, Any] = { - "query": query, - "limit": min(max_results, 100), # API limit per request - "fields": "paperId,title,abstract,year,url,openAccessPdf", - } - - headers = {} - if api_key: - headers["x-api-key"] = api_key - + """Sync papers for one source (restricted opencite search) into the DB.""" + opencite_source = _OPENCITE_SOURCE_BY_OSA[osa_source] try: - response = httpx.get(url, params=params, headers=headers, timeout=30.0) - response.raise_for_status() - data = response.json() - except httpx.HTTPStatusError as e: - logger.warning("Semantic Scholar HTTP error for '%s': %s", query, e) - return 0 - except httpx.RequestError as e: - logger.warning("Semantic Scholar request error for '%s': %s", query, e) + searched = _run(_search_queries(config, [query], max_results, (opencite_source,))) + except Exception as e: + logger.warning("opencite %s search failed for '%s': %s", osa_source, query, e) return 0 - count = 0 - with get_connection(project) as conn: - for paper in data.get("data", []): - if count >= max_results: - break - - # Skip if no title - title = paper.get("title") - if not title: - continue - - paper_id = paper.get("paperId", "") - paper_url = paper.get("url") or f"https://www.semanticscholar.org/paper/{paper_id}" + _, papers = searched[0] + counts = _store_papers(papers, project, force_source=osa_source) + count = sum(counts.values()) + logger.info("Synced %d papers from %s for '%s'", count, osa_source, query) + update_sync_metadata("papers", f"{osa_source}:{query}", count, project) + return count - # Prefer open access PDF URL if available - open_access = paper.get("openAccessPdf") - if open_access and open_access.get("url"): - paper_url = open_access["url"] - upsert_paper( - conn, - source="semanticscholar", - external_id=paper_id, - title=title, - first_message=paper.get("abstract"), - url=paper_url, - created_at=str(paper.get("year")) if paper.get("year") else None, - ) - count += 1 - - conn.commit() +def sync_openalex_papers(query: str, max_results: int = 100, project: str = "hed") -> int: + """Sync papers from OpenAlex matching query (via opencite).""" + logger.info("Syncing OpenAlex papers for query: %s", query) + return _sync_single_source(query, max_results, project, "openalex", _build_config()) - logger.info("Synced %d papers from Semantic Scholar for '%s'", count, query) - update_sync_metadata("papers", f"semanticscholar:{query}", count, project) - # Rate limiting - time.sleep(SEMANTIC_SCHOLAR_DELAY) - return count +def sync_semanticscholar_papers( + query: str, + max_results: int = 100, + api_key: str | None = None, + project: str = "hed", +) -> int: + """Sync papers from Semantic Scholar matching query (via opencite).""" + logger.info("Syncing Semantic Scholar papers for query: %s", query) + config = _build_config(semantic_scholar_api_key=api_key) + return _sync_single_source(query, max_results, project, "semanticscholar", config) def sync_pubmed_papers( @@ -261,109 +292,10 @@ def sync_pubmed_papers( api_key: str | None = None, project: str = "hed", ) -> int: - """Sync papers from PubMed matching query. - - Uses NCBI E-utilities API (esearch + efetch). - - Args: - query: Search query - max_results: Maximum number of papers to sync - api_key: Optional NCBI API key for higher rate limits - project: Assistant/project name for database isolation. Defaults to 'hed'. - - Returns: - Number of papers synced - """ + """Sync papers from PubMed matching query (via opencite).""" logger.info("Syncing PubMed papers for query: %s", query) - - base_url = "https://eutils.ncbi.nlm.nih.gov/entrez/eutils" - - # Step 1: Search for paper IDs - search_params: dict[str, Any] = { - "db": "pubmed", - "term": query, - "retmax": max_results, - "retmode": "json", - } - if api_key: - search_params["api_key"] = api_key - - try: - search_response = httpx.get(f"{base_url}/esearch.fcgi", params=search_params, timeout=30.0) - search_response.raise_for_status() - search_data = search_response.json() - except (httpx.HTTPStatusError, httpx.RequestError) as e: - logger.warning("PubMed search error for '%s': %s", query, e) - return 0 - - id_list = search_data.get("esearchresult", {}).get("idlist", []) - if not id_list: - logger.info("No PubMed results for '%s'", query) - return 0 - - # Rate limiting between requests - time.sleep(PUBMED_DELAY) - - # Step 2: Fetch paper details - fetch_params: dict[str, Any] = { - "db": "pubmed", - "id": ",".join(id_list), - "retmode": "xml", - } - if api_key: - fetch_params["api_key"] = api_key - - try: - fetch_response = httpx.get(f"{base_url}/efetch.fcgi", params=fetch_params, timeout=60.0) - fetch_response.raise_for_status() - except (httpx.HTTPStatusError, httpx.RequestError) as e: - logger.warning("PubMed fetch error for '%s': %s", query, e) - return 0 - - # Parse XML response - try: - root = ET.fromstring(fetch_response.text) - except ET.ParseError as e: - logger.warning("PubMed XML parse error for '%s': %s", query, e) - return 0 - - count = 0 - with get_connection(project) as conn: - for article in root.findall(".//PubmedArticle"): - pmid_elem = article.find(".//PMID") - title_elem = article.find(".//ArticleTitle") - abstract_elem = article.find(".//AbstractText") - year_elem = article.find(".//PubDate/Year") - - if pmid_elem is None or title_elem is None: - continue - - pmid = pmid_elem.text or "" - title = title_elem.text or "" - abstract = abstract_elem.text if abstract_elem is not None else None - year = year_elem.text if year_elem is not None else None - - url = f"https://pubmed.ncbi.nlm.nih.gov/{pmid}/" - - upsert_paper( - conn, - source="pubmed", - external_id=pmid, - title=title, - first_message=abstract, - url=url, - created_at=year, - ) - count += 1 - - conn.commit() - - logger.info("Synced %d papers from PubMed for '%s'", count, query) - update_sync_metadata("papers", f"pubmed:{query}", count, project) - - # Rate limiting - time.sleep(PUBMED_DELAY) - return count + config = _build_config(pubmed_api_key=api_key) + return _sync_single_source(query, max_results, project, "pubmed", config) def sync_all_papers( @@ -375,19 +307,23 @@ def sync_all_papers( openalex_email: str | None = None, project: str = "hed", ) -> dict[str, int]: - """Sync papers from all sources for given queries. + """Sync papers from all default sources for given queries via opencite. + + A single deduplicated opencite search runs per query across + ``DEFAULT_SOURCES``, replacing the previous three sequential per-source + fetches. Args: - queries: List of search queries (required - no default queries) - max_results: Max results per query per source - semantic_scholar_api_key: Optional Semantic Scholar API key - pubmed_api_key: Optional PubMed/NCBI API key - openalex_api_key: Optional OpenAlex API key for premium access - openalex_email: Optional email for OpenAlex polite pool - project: Project/community ID for database isolation + queries: List of search queries (required - no default queries). + max_results: Max deduplicated results per query. + semantic_scholar_api_key: Optional Semantic Scholar API key. + pubmed_api_key: Optional PubMed/NCBI API key. + openalex_api_key: Optional OpenAlex API key for premium access. + openalex_email: Optional email for OpenAlex polite pool. + project: Project/community ID for database isolation. Returns: - Dict mapping source to total items synced + Dict mapping OSA source label to total papers synced. """ if isinstance(queries, str): raise TypeError(f"queries must be a list of strings, not a bare string: {queries!r}") @@ -395,21 +331,25 @@ def sync_all_papers( logger.warning("No queries provided for paper sync") return {"openalex": 0, "semanticscholar": 0, "pubmed": 0} - # Configure OpenAlex with API key or email if provided - configure_openalex(api_key=openalex_api_key, email=openalex_email) + config = _build_config( + openalex_api_key=openalex_api_key, + openalex_email=openalex_email, + semantic_scholar_api_key=semantic_scholar_api_key, + pubmed_api_key=pubmed_api_key, + ) - results = { - "openalex": 0, - "semanticscholar": 0, - "pubmed": 0, - } + results: dict[str, int] = {"openalex": 0, "semanticscholar": 0, "pubmed": 0} + try: + searched = _run(_search_queries(config, queries, max_results, DEFAULT_SOURCES)) + except Exception as e: + logger.warning("opencite search failed for %s: %s", project, e) + return results - for query in queries: - results["openalex"] += sync_openalex_papers(query, max_results, project=project) - results["semanticscholar"] += sync_semanticscholar_papers( - query, max_results, semantic_scholar_api_key, project=project - ) - results["pubmed"] += sync_pubmed_papers(query, max_results, pubmed_api_key, project=project) + for query, papers in searched: + counts = _store_papers(papers, project) + for source, n in counts.items(): + results[source] = results.get(source, 0) + n + update_sync_metadata("papers", f"opencite:{query}", sum(counts.values()), project) total = sum(results.values()) logger.info("Total papers synced for %s: %d", project, total) @@ -423,92 +363,35 @@ def sync_citing_papers( openalex_api_key: str | None = None, openalex_email: str | None = None, ) -> int: - """Sync papers that cite the given DOIs using OpenALEX. - - OpenALEX supports finding papers that cite a specific work via - the `cites` filter. This is useful for tracking citations to - foundational papers in a field. + """Sync papers that cite the given DOIs using opencite's citation graph. Args: - dois: List of DOIs to find citations for. Should be in bare format - (e.g., "10.1016/j.neuroimage.2021.118809") without the - https://doi.org/ prefix. Invalid or unfound DOIs are skipped - with a warning log. - max_results: Maximum number of citing papers per DOI - project: Project/community ID for database isolation - openalex_api_key: Optional OpenAlex API key for premium access - openalex_email: Optional email for OpenAlex polite pool + dois: List of DOIs to find citations for. Bare format preferred + (e.g. "10.1016/j.neuroimage.2021.118809"); opencite auto-detects + and resolves the identifier. Unresolved DOIs are skipped with a + warning. + max_results: Maximum number of citing papers per DOI. + project: Project/community ID for database isolation. + openalex_api_key: Optional OpenAlex API key for premium access. + openalex_email: Optional email for OpenAlex polite pool. Returns: - Total number of citing papers synced + Total number of citing papers synced. """ if isinstance(dois, str): raise TypeError(f"dois must be a list of strings, not a bare string: {dois!r}") - configure_openalex(api_key=openalex_api_key, email=openalex_email) - total = 0 - for doi in dois: - logger.info("Syncing papers citing DOI: %s", doi) - - try: - # First, look up the OpenALEX work ID for this DOI - work_lookup = Works()[f"https://doi.org/{doi}"] - openalex_id = work_lookup.get("id") - - if not openalex_id: - logger.warning("Could not find OpenALEX ID for DOI %s", doi) - continue + config = _build_config(openalex_api_key=openalex_api_key, openalex_email=openalex_email) + try: + cited = _run(_citing_for_dois(config, dois, max_results)) + except Exception as e: + logger.warning("opencite citation lookup failed for %s: %s", project, e) + return 0 - logger.debug("Found OpenALEX ID %s for DOI %s", openalex_id, doi) - - # Now find papers that cite this work using the OpenALEX ID - works_query = ( - Works() - .filter(cites=openalex_id) - .select( - [ - "id", - "title", - "abstract_inverted_index", - "publication_date", - "doi", - "primary_location", - ] - ) - ) - works = list(works_query.get(per_page=min(max_results, 200))) - except Exception as e: - logger.warning("OpenALEX citation error for DOI %s: %s", doi, e) - continue - - count = 0 - with get_connection(project) as conn: - for work in works: - if count >= max_results: - break - - title = work.get("title") - if not title: - continue - - abstract = _reconstruct_abstract(work.get("abstract_inverted_index")) - url = _get_paper_url(work.get("doi"), work.get("id", "")) - external_id = _get_openalex_external_id(work.get("id", "")) - - upsert_paper( - conn, - source="openalex", - external_id=external_id, - title=title, - first_message=abstract, - url=url, - created_at=work.get("publication_date"), - ) - count += 1 - - conn.commit() - - # Update sync metadata with citing_ prefix to distinguish from query-based syncs + total = 0 + for doi, papers in cited: + counts = _store_papers(papers, project) + count = sum(counts.values()) update_sync_metadata("papers", f"citing_{doi}", count, project) logger.info("Synced %d papers citing %s", count, doi) total += count diff --git a/tests/test_knowledge/test_papers_sync.py b/tests/test_knowledge/test_papers_sync.py index ab2d123..323d0f4 100644 --- a/tests/test_knowledge/test_papers_sync.py +++ b/tests/test_knowledge/test_papers_sync.py @@ -1,17 +1,23 @@ -"""Tests for papers sync module. +"""Tests for the opencite-backed papers sync module. -Note: These are real API tests, not mocks, per project guidelines. +Mapping tests use real opencite ``Paper`` objects and a real SQLite database +(no mocks). The sync smoke tests make real network calls per project +guidelines. """ +import asyncio from pathlib import Path from unittest.mock import patch -import pyalex import pytest +from opencite import IDSet, Paper +import src.knowledge.papers_sync as ps from src.knowledge.db import get_connection, init_db from src.knowledge.papers_sync import ( - _reconstruct_abstract, + _paper_source_and_id, + _paper_url, + _store_papers, configure_openalex, sync_all_papers, sync_citing_papers, @@ -29,117 +35,181 @@ def temp_db(tmp_path: Path): class TestConfigureOpenalex: - """Tests for configure_openalex helper.""" + """Tests for the configure_openalex credential helper.""" def setup_method(self): - """Reset pyalex config before each test.""" - pyalex.config.api_key = None - pyalex.config.email = None + """Reset stored OpenAlex credentials before each test.""" + configure_openalex(api_key=None, email=None) def teardown_method(self): - """Reset pyalex config after each test.""" - pyalex.config.api_key = None - pyalex.config.email = None + """Reset stored OpenAlex credentials after each test.""" + configure_openalex(api_key=None, email=None) def test_sets_api_key(self): - """Should set pyalex.config.api_key when api_key provided.""" configure_openalex(api_key="test-key-123") - assert pyalex.config.api_key == "test-key-123" + assert ps._OPENALEX_API_KEY == "test-key-123" - def test_sets_email_when_no_api_key(self): - """Should set pyalex.config.email when only email provided.""" + def test_sets_email(self): configure_openalex(email="test@example.com") - assert pyalex.config.email == "test@example.com" + assert ps._OPENALEX_EMAIL == "test@example.com" + assert ps._OPENALEX_API_KEY is None - def test_api_key_takes_precedence_over_email(self): - """Should use API key over email when both provided.""" + def test_sets_both_key_and_email(self): configure_openalex(api_key="test-key", email="test@example.com") - assert pyalex.config.api_key == "test-key" + assert ps._OPENALEX_API_KEY == "test-key" + assert ps._OPENALEX_EMAIL == "test@example.com" def test_handles_empty_strings(self): - """Should treat empty strings as None (no config).""" configure_openalex(api_key="", email="") - assert pyalex.config.api_key is None - assert pyalex.config.email is None + assert ps._OPENALEX_API_KEY is None + assert ps._OPENALEX_EMAIL is None def test_handles_whitespace_strings(self): - """Should strip whitespace and treat blank as None.""" configure_openalex(api_key=" ", email=" ") - assert pyalex.config.api_key is None - assert pyalex.config.email is None + assert ps._OPENALEX_API_KEY is None + assert ps._OPENALEX_EMAIL is None def test_handles_none_values(self): - """Should handle None values gracefully (anonymous access).""" configure_openalex(api_key=None, email=None) - assert pyalex.config.api_key is None - assert pyalex.config.email is None - - -class TestAbstractReconstruction: - """Test OpenALEX inverted index reconstruction.""" - - def test_reconstruct_abstract_basic(self): - """Test basic abstract reconstruction from inverted index.""" - inverted_index = { - "hello": [0], - "world": [1], - } - result = _reconstruct_abstract(inverted_index) - assert "hello" in result - assert "world" in result - - def test_reconstruct_abstract_with_gaps(self): - """Test reconstruction with gaps in position array.""" - inverted_index = { - "hello": [0], - "world": [2], # Missing position 1 - } - result = _reconstruct_abstract(inverted_index) - # Should handle gaps gracefully (empty string at position 1) - assert "hello" in result - assert "world" in result - - def test_reconstruct_abstract_empty(self): - """Test reconstruction with empty/None input.""" - assert _reconstruct_abstract(None) == "" - assert _reconstruct_abstract({}) == "" - - def test_reconstruct_abstract_complex(self): - """Test reconstruction with longer text.""" - inverted_index = { - "Hierarchical": [0], - "Event": [1], - "Descriptors": [2], - "(HED)": [3], - "is": [4], - "a": [5], - "framework": [6], - } - result = _reconstruct_abstract(inverted_index) - expected_words = ["Hierarchical", "Event", "Descriptors", "HED", "framework"] - for word in expected_words: - assert word in result + assert ps._OPENALEX_API_KEY is None + assert ps._OPENALEX_EMAIL is None + + +class TestPaperMapping: + """Map opencite Paper objects to (source, external_id) and URLs.""" + + def test_prefers_openalex_id(self): + paper = Paper( + title="X", + ids=IDSet(openalex_id="https://openalex.org/W7", doi="10.1/A", pmid="9"), + ) + assert _paper_source_and_id(paper) == ("openalex", "W7") + + def test_falls_back_to_semantic_scholar(self): + paper = Paper(title="Y", ids=IDSet(s2_id="S99")) + assert _paper_source_and_id(paper) == ("semanticscholar", "S99") + + def test_falls_back_to_pubmed(self): + paper = Paper(title="Y", ids=IDSet(pmid="12345")) + assert _paper_source_and_id(paper) == ("pubmed", "12345") + + def test_falls_back_to_doi_lowercased(self): + paper = Paper(title="Y", ids=IDSet(doi="10.1/AbC")) + assert _paper_source_and_id(paper) == ("doi", "10.1/abc") + + def test_falls_back_to_arxiv(self): + paper = Paper(title="Y", ids=IDSet(arxiv_id="2106.15928")) + assert _paper_source_and_id(paper) == ("arxiv", "2106.15928") + + def test_no_identifier_is_skipped(self): + paper = Paper(title="orphan", ids=IDSet()) + assert _paper_source_and_id(paper) == (None, None) + + def test_url_prefers_doi_landing_page(self): + paper = Paper(title="X", ids=IDSet(doi="10.1/A"), url="https://openalex.org/W7") + assert _paper_url(paper) == "https://doi.org/10.1/A" + + def test_url_falls_back_to_paper_url(self): + paper = Paper(title="X", ids=IDSet(), url="https://example.org/p") + assert _paper_url(paper) == "https://example.org/p" + + def test_url_empty_when_nothing_available(self): + paper = Paper(title="X", ids=IDSet()) + assert _paper_url(paper) == "" + + +class TestStorePapers: + """Persist opencite papers into the knowledge DB (real SQLite, no mocks).""" + + def test_stores_and_labels_sources(self, temp_db: Path): + papers = [ + Paper( + title="EEGLAB toolbox", + ids=IDSet(openalex_id="https://openalex.org/W1", doi="10.1/eeglab"), + year=2004, + abstract="An open source toolbox.", + ), + Paper(title="S2 paper", ids=IDSet(s2_id="S2"), year=2020), + ] + with patch("src.knowledge.db.get_db_path", return_value=temp_db): + counts = _store_papers(papers, "test") + + assert counts == {"openalex": 1, "semanticscholar": 1} + with get_connection("test") as conn: + rows = { + r["source"]: r + for r in conn.execute("SELECT source, external_id, url, title FROM papers") + } + assert rows["openalex"]["external_id"] == "W1" + assert rows["openalex"]["url"] == "https://doi.org/10.1/eeglab" + assert rows["semanticscholar"]["external_id"] == "S2" + + def test_skips_papers_without_title_or_id(self, temp_db: Path): + papers = [ + Paper(title="", ids=IDSet(openalex_id="https://openalex.org/W1")), + Paper(title="no id", ids=IDSet()), + ] + with patch("src.knowledge.db.get_db_path", return_value=temp_db): + counts = _store_papers(papers, "test") + assert counts == {} + + def test_upsert_deduplicates_same_paper(self, temp_db: Path): + paper = Paper(title="dup", ids=IDSet(openalex_id="https://openalex.org/W1"), year=2020) + with patch("src.knowledge.db.get_db_path", return_value=temp_db): + _store_papers([paper], "test") + _store_papers([paper], "test") + with get_connection("test") as conn: + count = conn.execute("SELECT COUNT(*) AS c FROM papers").fetchone()["c"] + assert count == 1 + + def test_force_source_uses_native_id(self, temp_db: Path): + # A PubMed-restricted sync should label the row 'pubmed' using the PMID, + # even though the paper also carries an OpenAlex id. + paper = Paper( + title="P", + ids=IDSet(openalex_id="https://openalex.org/W1", pmid="555"), + ) + with patch("src.knowledge.db.get_db_path", return_value=temp_db): + counts = _store_papers([paper], "test", force_source="pubmed") + assert counts == {"pubmed": 1} + with get_connection("test") as conn: + row = conn.execute("SELECT source, external_id FROM papers").fetchone() + assert row["source"] == "pubmed" + assert row["external_id"] == "555" + + +async def _answer() -> int: + return 42 + + +class TestRunHelper: + """The _run async bridge must work with or without a running event loop.""" + + def test_runs_without_existing_loop(self): + # Sync context (CLI / scheduler thread): asyncio.run path. + assert ps._run(_answer()) == 42 + + def test_runs_inside_running_loop(self): + # If a loop is already running, _run offloads to a worker thread instead + # of raising "asyncio.run() cannot be called from a running event loop". + async def driver() -> int: + return ps._run(_answer()) + + assert asyncio.run(driver()) == 42 class TestPapersSync: - """Test papers sync functionality.""" + """Smoke tests using real opencite/network calls.""" def test_sync_openalex_papers_basic(self, temp_db: Path): - """Test basic OpenALEX papers sync. - - This is a smoke test using a real OpenALEX API call. - """ + """Basic OpenAlex sync through opencite (real API call).""" with patch("src.knowledge.db.get_db_path", return_value=temp_db): - # Sync a small number of papers with a specific query count = sync_openalex_papers( "Hierarchical Event Descriptors", max_results=5, project="test" ) - # Should find at least some results (OpenALEX doesn't require auth) - # Accept 0 for network issues + # Accept 0 for transient network issues. assert count >= 0 - - # If count > 0, verify data was written if count > 0: with get_connection("test") as conn: row = conn.execute( @@ -147,27 +217,15 @@ def test_sync_openalex_papers_basic(self, temp_db: Path): ).fetchone() assert row["count"] > 0 - def test_sync_openalex_papers_no_results(self, temp_db: Path): - """Test OpenALEX sync with query that returns no results.""" - with patch("src.knowledge.db.get_db_path", return_value=temp_db): - # Use an extremely specific nonsense query - count = sync_openalex_papers("xyzabc123nonsensequery", max_results=5, project="test") - - # Should return 0 for no results (not an error) - assert count == 0 - def test_sync_respects_max_results(self, temp_db: Path): - """Test that max_results parameter is respected.""" + """max_results is respected for a single-source sync.""" with patch("src.knowledge.db.get_db_path", return_value=temp_db): - # Request only 2 results count = sync_openalex_papers("neuroscience", max_results=2, project="test") - - # Should not exceed max_results assert count <= 2 class TestPapersSyncTypeGuard: - """Test that sync functions reject bare strings to prevent character iteration.""" + """Sync functions reject bare strings to prevent character iteration.""" def test_sync_all_papers_rejects_bare_string(self) -> None: with pytest.raises(TypeError, match="must be a list of strings"): diff --git a/uv.lock b/uv.lock index 22d7aee..6995cbd 100644 --- a/uv.lock +++ b/uv.lock @@ -2040,6 +2040,7 @@ dev = [ { name = "lxml" }, { name = "markdownify" }, { name = "mypy" }, + { name = "opencite" }, { name = "pre-commit" }, { name = "psycopg", extra = ["binary"] }, { name = "pyalex" }, @@ -2071,6 +2072,7 @@ server = [ { name = "litellm" }, { name = "lxml" }, { name = "markdownify" }, + { name = "opencite" }, { name = "psycopg", extra = ["binary"] }, { name = "pyalex" }, { name = "pydantic-settings" }, @@ -2112,6 +2114,8 @@ requires-dist = [ { name = "markdownify", marker = "extra == 'dev'", specifier = ">=1.1.0" }, { name = "markdownify", marker = "extra == 'server'", specifier = ">=1.1.0" }, { name = "mypy", marker = "extra == 'dev'", specifier = ">=1.19.0" }, + { name = "opencite", marker = "extra == 'dev'", specifier = ">=0.5.2" }, + { name = "opencite", marker = "extra == 'server'", specifier = ">=0.5.2" }, { name = "platformdirs", specifier = ">=4.5.0" }, { name = "pre-commit", marker = "extra == 'dev'", specifier = ">=4.5.0" }, { name = "psycopg", extras = ["binary"], marker = "extra == 'dev'", specifier = ">=3.3.0" }, @@ -2157,6 +2161,19 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/27/4b/7c1a00c2c3fbd004253937f7520f692a9650767aa73894d7a34f0d65d3f4/openai-2.14.0-py3-none-any.whl", hash = "sha256:7ea40aca4ffc4c4a776e77679021b47eec1160e341f42ae086ba949c9dcc9183", size = 1067558, upload-time = "2025-12-19T03:28:43.727Z" }, ] +[[package]] +name = "opencite" +version = "0.5.2" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "httpx" }, + { name = "pyalex" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/7a/18/313a129d8cee89784c409ec19049f09adf6ba515792747b9dd65dbbd9ddb/opencite-0.5.2.tar.gz", hash = "sha256:d7a63482e4d1a0372fd2d01cc246e3d10b74d50393ae153afe53029b490e7d7e", size = 142084, upload-time = "2026-05-21T19:54:15.971Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/0e/b8/d00880ff96e9d3808c6b65e6347b7c4f9fdc8b72213a5dd4cd147224207e/opencite-0.5.2-py3-none-any.whl", hash = "sha256:d4ffb6b553c94df454dbc1de4eed55fdc2ea7604099fea0e3fdbc5a5ea654ce1", size = 96732, upload-time = "2026-05-21T19:54:14.574Z" }, +] + [[package]] name = "opentelemetry-api" version = "1.39.1" From b016e3c36a64c729a052a3099e68a8f866c27fe4 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 5 Jun 2026 07:49:20 +0000 Subject: [PATCH 29/39] Bump version to 0.8.4.dev14 --- src/version.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/version.py b/src/version.py index 7ea87eb..eda7351 100644 --- a/src/version.py +++ b/src/version.py @@ -1,7 +1,7 @@ """Version information for OSA.""" -__version__ = "0.8.4.dev13" -__version_info__ = (0, 8, 4, "dev13") +__version__ = "0.8.4.dev14" +__version_info__ = (0, 8, 4, "dev14") def get_version() -> str: From da68367282eb7ae45a34617d852deb2c05da0af6 Mon Sep 17 00:00:00 2001 From: "Seyed (Yahya) Shirazi" Date: Fri, 5 Jun 2026 01:34:32 -0700 Subject: [PATCH 30/39] feat(papers): live on-demand 'search recent papers' via opencite (#310) * feat(papers): live on-demand paper search via opencite Adds search__papers_live, an opt-in tool that queries opencite for the most recent literature (newest first) when the user asks for recent/new papers or the local index comes up short. Results are best-effort cached into the community DB so future local searches find them. Bounded by a timeout to keep chat responsive. - search_papers_live() in papers_sync.py reuses the opencite client + _run async bridge; keys read from the server's env. - Gated by citations.live_search (new config flag, default on) wherever paper citations are configured. - EEGLAB prompt guides the agent to use it for recency. Closes #308 * fix(papers): non-blocking cache + cleaner live-search timeout Address PR review: - cache live-search results in a background daemon thread so a chat response is never delayed (was synchronous; could block up to the SQLite busy timeout if the scheduler was mid-sync on the same DB). - set opencite per-request timeout just under the overall cap so each source finishes/times out cleanly before wait_for cancels, avoiding orphaned-task error noise on the timeout path. - test caching deterministically via _cache_papers_async (offline, real SQLite); live network test now just asserts result shape. --- src/assistants/community.py | 4 + src/assistants/eeglab/config.yaml | 5 +- src/core/config/community.py | 3 + src/knowledge/papers_sync.py | 118 +++++++++++++++++++++++ src/tools/knowledge.py | 63 ++++++++++++ tests/test_knowledge/test_papers_sync.py | 63 ++++++++++++ tests/test_tools/test_knowledge_tools.py | 12 +++ 7 files changed, 267 insertions(+), 1 deletion(-) diff --git a/src/assistants/community.py b/src/assistants/community.py index c65860a..620d923 100644 --- a/src/assistants/community.py +++ b/src/assistants/community.py @@ -225,6 +225,9 @@ def _build_tools(self, config: CommunityConfig) -> list[BaseTool]: repos = config.github.repos if config.github else None has_github = config.github and config.github.repos has_citations = config.citations and (config.citations.queries or config.citations.dois) + has_live_papers = ( + bool(has_citations) and config.citations is not None and config.citations.live_search + ) has_docstrings = config.docstrings and config.docstrings.repos has_faq = config.faq_generation is not None and bool(config.mailman) @@ -237,6 +240,7 @@ def _build_tools(self, config: CommunityConfig) -> list[BaseTool]: include_discussions=bool(has_github), include_recent=bool(has_github), include_papers=bool(has_citations), + include_live_papers=has_live_papers, include_docstrings=bool(has_docstrings), include_faq=bool(has_faq), faq_list_names=([m.list_name for m in config.mailman] if config.mailman else None), diff --git a/src/assistants/eeglab/config.yaml b/src/assistants/eeglab/config.yaml index 62708c4..7c67128 100644 --- a/src/assistants/eeglab/config.yaml +++ b/src/assistants/eeglab/config.yaml @@ -146,7 +146,8 @@ system_prompt: | 5. `search_eeglab_faq`: Search mailing list Q&A (archives since 2004) **Research:** - 6. `search_eeglab_papers`: Search academic literature about EEGLAB and EEG analysis + 6. `search_eeglab_papers`: Search already-indexed academic literature about EEGLAB and EEG analysis + 7. `search_eeglab_papers_live`: Live on-demand search of the latest literature (newest first) when the user asks for recent/new papers or the indexed search comes up short ## Tool Usage Guidelines @@ -196,6 +197,8 @@ system_prompt: | - "Has anyone cited the EEGLAB paper?" -> CALL `search_eeglab_papers(query="EEGLAB")` - "Papers about ICA in EEGLAB?" -> CALL `search_eeglab_papers(query="ICA EEGLAB")` - "Research on ICLabel?" -> CALL `search_eeglab_papers(query="ICLabel")` + - "What are the latest/newest papers on ICLabel?" -> CALL `search_eeglab_papers_live(query="ICLabel")` + - "Any recent papers using clean_rawdata?" -> CALL `search_eeglab_papers_live(query="clean_rawdata ASR EEG")` **DO NOT:** - Tell users to "visit GitHub", "check Google Scholar", or "use the API" when you have the data diff --git a/src/core/config/community.py b/src/core/config/community.py index a3ab38f..c7bce15 100644 --- a/src/core/config/community.py +++ b/src/core/config/community.py @@ -236,6 +236,9 @@ class CitationConfig(BaseModel): dois: list[str] = Field(default_factory=list) """Core paper DOIs to track citations for (format: '10.xxxx/yyyy').""" + live_search: bool = Field(default=True) + """Expose an on-demand live paper search tool (opencite) for recent literature.""" + @field_validator("queries") @classmethod def validate_queries(cls, v: list[str]) -> list[str]: diff --git a/src/knowledge/papers_sync.py b/src/knowledge/papers_sync.py index 640ad4c..b981207 100644 --- a/src/knowledge/papers_sync.py +++ b/src/knowledge/papers_sync.py @@ -14,6 +14,8 @@ import asyncio import logging +import os +import threading from collections.abc import Iterable from concurrent.futures import ThreadPoolExecutor @@ -22,6 +24,7 @@ from opencite.search import SearchOrchestrator from src.knowledge.db import get_connection, update_sync_metadata, upsert_paper +from src.knowledge.search import SearchResult logger = logging.getLogger(__name__) @@ -397,3 +400,118 @@ def sync_citing_papers( total += count return total + + +def _config_from_env() -> Config: + """Build an opencite Config from the server's configured API-key env vars. + + Reads the same variables OSA settings use. Missing keys fall back to + anonymous access (fine for a single on-demand query). Specific env vars + are read by name rather than via Config.from_env() to avoid the ambient + ``.env`` parsing path. + """ + return _build_config( + openalex_api_key=os.environ.get("OPENALEX_API_KEY"), + openalex_email=os.environ.get("OPENALEX_EMAIL"), + semantic_scholar_api_key=os.environ.get("SEMANTIC_SCHOLAR_API_KEY"), + pubmed_api_key=os.environ.get("PUBMED_API_KEY"), + ) + + +def _paper_to_result(paper: Paper) -> SearchResult: + """Convert an opencite Paper to the shared SearchResult shape.""" + source, _ = _paper_source_and_id(paper) + return SearchResult( + title=paper.title, + url=_paper_url(paper), + snippet=paper.abstract or "", + source=source or "opencite", + item_type=None, + status="published", + created_at=str(paper.year) if paper.year else "", + ) + + +async def _search_recent( + config: Config, + query: str, + limit: int, + timeout: float, +) -> list[Paper]: + """Live opencite search for the most recent papers, bounded by a timeout. + + The per-request timeout (``config.timeout``, set by the caller) is the + primary bound and is kept just under ``timeout`` so each source finishes or + times out cleanly before the outer ``wait_for`` would have to cancel and + orphan opencite's in-flight tasks. + """ + async with SearchOrchestrator(config) as searcher: + result = await asyncio.wait_for( + searcher.search(query, max_results=limit, sources=DEFAULT_SOURCES, sort="year"), + timeout=timeout, + ) + return result.papers + + +def _cache_papers_async(papers: list[Paper], project: str) -> threading.Thread: + """Cache live-search results into the DB without blocking the caller. + + Caching is best-effort: it must never add latency to (or fail) the chat + response, so the write runs in a daemon thread and logs on error. Returns + the thread (useful for tests). + """ + + def _write() -> None: + try: + _store_papers(papers, project) + except Exception as e: + logger.warning("Failed to cache live search papers for %s: %s", project, e) + + thread = threading.Thread(target=_write, name=f"cache-papers-{project}", daemon=True) + thread.start() + return thread + + +def search_papers_live( + query: str, + project: str = "hed", + limit: int = 5, + cache: bool = True, + timeout: float = 20.0, +) -> list[SearchResult]: + """Search the live literature via opencite for the most recent papers. + + Unlike :func:`src.knowledge.search.search_papers` (local FTS over already + synced rows), this hits opencite's multi-source APIs for fresh results, + newest first. This is for on-demand discovery of papers the batch sync has + not picked up yet. + + Args: + query: Topic to search for. + project: Community/project ID (for caching into the right DB). + limit: Maximum number of papers to return. + cache: When True (default), best-effort upsert the results into the + community knowledge DB (in a background thread, never blocking the + response) so future local searches find them. + timeout: Hard cap (seconds) on the opencite call to keep chat snappy. + + Returns: + List of SearchResult, newest first. Empty on timeout/error. + """ + config = _config_from_env() + # Bound each source request just under the overall cap so opencite's + # per-source tasks finish cleanly before wait_for would cancel them. + config.timeout = max(1.0, timeout - 2.0) + try: + papers = _run(_search_recent(config, query, limit, timeout)) + except TimeoutError: + logger.warning("opencite live search timed out for '%s' after %.0fs", query, timeout) + return [] + except Exception as e: + logger.warning("opencite live search failed for '%s': %s", query, e) + return [] + + if cache and papers: + _cache_papers_async(papers, project) + + return [_paper_to_result(p) for p in papers[:limit]] diff --git a/src/tools/knowledge.py b/src/tools/knowledge.py index 8f43258..d387570 100644 --- a/src/tools/knowledge.py +++ b/src/tools/knowledge.py @@ -22,6 +22,7 @@ from langchain_core.tools import BaseTool, StructuredTool from src.knowledge.db import get_db_path +from src.knowledge.papers_sync import search_papers_live from src.knowledge.search import ( get_full_docstring, list_recent_github_items, @@ -256,6 +257,63 @@ def search_papers_impl(query: str, limit: int = 5) -> str: ) +def create_search_papers_live_tool( + community_id: str, + community_name: str, +) -> BaseTool: + """Create a tool for live (on-demand) academic paper search via opencite. + + Unlike the local paper search (pre-synced rows), this fetches fresh results + from the live literature, newest first, and caches them for next time. + + Args: + community_id: The community identifier (e.g., 'hed', 'eeglab') + community_name: Display name (e.g., 'HED', 'EEGLAB') + + Returns: + A LangChain tool for live paper search + """ + + def search_papers_live_impl(query: str, limit: int = 5) -> str: + """Live academic paper search implementation.""" + results = search_papers_live(query, project=community_id, limit=limit) + + if not results: + return ( + f"No recent papers found online for '{query}' " + "(the live search may have timed out; try the local paper search)." + ) + + lines = ["Most recent papers (live search):\n"] + for r in results: + year = f" ({r.created_at})" if r.created_at else "" + source_label = f"[{r.source}]" if r.source else "" + lines.append(f"- {r.title}{year} {source_label}") + lines.append(f" [View Paper]({r.url})") + if r.snippet: + snippet = r.snippet[:200] + "..." if len(r.snippet) > 200 else r.snippet + lines.append(f" Abstract: {snippet}") + lines.append("") + + return "\n".join(lines) + + description = ( + f"Search the LIVE literature for the most recent papers about {community_name}. " + "**Use this only when the user explicitly asks for recent / latest / new papers, " + "or when the local paper search returns nothing relevant.** " + "It queries external sources on demand (slower than the local search) and returns " + "the newest results first. " + "**This is for DISCOVERY, not answering** - present results as references for " + "further reading; do NOT use paper content to formulate answers." + ) + + return StructuredTool.from_function( + func=search_papers_live_impl, + name=f"search_{community_id}_papers_live", + description=description, + ) + + def create_search_docstrings_tool( community_id: str, community_name: str, @@ -611,6 +669,7 @@ def create_knowledge_tools( include_discussions: bool = True, include_recent: bool = True, include_papers: bool = True, + include_live_papers: bool = False, include_docstrings: bool = False, docstrings_language: str | None = None, include_faq: bool = False, @@ -629,6 +688,7 @@ def create_knowledge_tools( include_discussions: Include discussion search tool (default: True) include_recent: Include recent activity tool (default: True) include_papers: Include paper search tool (default: True) + include_live_papers: Include on-demand live paper search tool (default: False) include_docstrings: Include code docstring search tool (default: False) docstrings_language: Filter docstrings by language ('matlab' or 'python') include_faq: Include mailing list FAQ search tool (default: False) @@ -649,6 +709,9 @@ def create_knowledge_tools( if include_papers: tools.append(create_search_papers_tool(community_id, community_name)) + if include_live_papers: + tools.append(create_search_papers_live_tool(community_id, community_name)) + if include_docstrings: tools.append( create_search_docstrings_tool(community_id, community_name, docstrings_language) diff --git a/tests/test_knowledge/test_papers_sync.py b/tests/test_knowledge/test_papers_sync.py index 323d0f4..e6cbde2 100644 --- a/tests/test_knowledge/test_papers_sync.py +++ b/tests/test_knowledge/test_papers_sync.py @@ -15,10 +15,13 @@ import src.knowledge.papers_sync as ps from src.knowledge.db import get_connection, init_db from src.knowledge.papers_sync import ( + _cache_papers_async, _paper_source_and_id, + _paper_to_result, _paper_url, _store_papers, configure_openalex, + search_papers_live, sync_all_papers, sync_citing_papers, sync_openalex_papers, @@ -224,6 +227,66 @@ def test_sync_respects_max_results(self, temp_db: Path): assert count <= 2 +class TestPaperToResult: + """Map opencite Paper objects to the shared SearchResult shape.""" + + def test_maps_core_fields(self): + paper = Paper( + title="Recent EEG paper", + ids=IDSet(openalex_id="https://openalex.org/W9", doi="10.1/x"), + year=2026, + abstract="Latest findings.", + ) + result = _paper_to_result(paper) + assert result.title == "Recent EEG paper" + assert result.url == "https://doi.org/10.1/x" + assert result.source == "openalex" + assert result.created_at == "2026" + assert result.status == "published" + assert result.snippet == "Latest findings." + + def test_handles_missing_year_and_id(self): + result = _paper_to_result(Paper(title="No metadata", ids=IDSet())) + assert result.created_at == "" + assert result.source == "opencite" + + +class TestCachePapersAsync: + """Background caching of live-search results (real SQLite, no mocks).""" + + def test_caches_papers_into_db(self, temp_db: Path): + papers = [ + Paper( + title="Cached paper", ids=IDSet(openalex_id="https://openalex.org/W5"), year=2026 + ), + ] + with patch("src.knowledge.db.get_db_path", return_value=temp_db): + # Caching is async; join the returned thread before asserting. + _cache_papers_async(papers, "test").join(timeout=10) + with get_connection("test") as conn: + count = conn.execute("SELECT COUNT(*) AS c FROM papers").fetchone()["c"] + assert count == 1 + + +class TestLivePaperSearch: + """Live opencite search (real network).""" + + def test_live_search_returns_recent(self, temp_db: Path): + with patch("src.knowledge.db.get_db_path", return_value=temp_db): + results = search_papers_live( + "EEGLAB EEG independent component analysis", + project="test", + limit=3, + timeout=40, + ) + + # Network-dependent: accept empty on transient failure, but the shape + # must always be correct and every result must be displayable. + assert isinstance(results, list) + assert all(r.status == "published" for r in results) + assert all(r.title for r in results) + + class TestPapersSyncTypeGuard: """Sync functions reject bare strings to prevent character iteration.""" diff --git a/tests/test_tools/test_knowledge_tools.py b/tests/test_tools/test_knowledge_tools.py index c1cd39a..682bc9b 100644 --- a/tests/test_tools/test_knowledge_tools.py +++ b/tests/test_tools/test_knowledge_tools.py @@ -278,6 +278,18 @@ def test_includes_docstrings_tool_when_enabled(self) -> None: assert "get_test_full_docstring" in tool_names assert len(tools) == 5 + def test_excludes_live_papers_by_default(self) -> None: + """Live paper search is opt-in and absent unless requested.""" + tools = create_knowledge_tools("test", "Test") + assert "search_test_papers_live" not in [t.name for t in tools] + + def test_includes_live_papers_tool_when_enabled(self) -> None: + """Should include the live paper search tool when include_live_papers=True.""" + tools = create_knowledge_tools("test", "Test", include_live_papers=True) + tool_names = [t.name for t in tools] + assert "search_test_papers_live" in tool_names + assert len(tools) == 4 + def test_includes_faq_tool_when_enabled(self) -> None: """Should include FAQ search tool when include_faq=True.""" tools = create_knowledge_tools("test", "Test", include_faq=True) From 226dc8272f64d39da04424297826d443297e6697 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 5 Jun 2026 08:34:48 +0000 Subject: [PATCH 31/39] Bump version to 0.8.4.dev15 --- src/version.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/version.py b/src/version.py index eda7351..d3ebbe8 100644 --- a/src/version.py +++ b/src/version.py @@ -1,7 +1,7 @@ """Version information for OSA.""" -__version__ = "0.8.4.dev14" -__version_info__ = (0, 8, 4, "dev14") +__version__ = "0.8.4.dev15" +__version_info__ = (0, 8, 4, "dev15") def get_version() -> str: From 714e44f5c1041366de54406d5e1592e543ac4b1b Mon Sep 17 00:00:00 2001 From: "Seyed (Yahya) Shirazi" Date: Fri, 5 Jun 2026 02:15:02 -0700 Subject: [PATCH 32/39] feat(papers): local-first live search, confirmed + faster (#313) * feat(papers): local-first live search, confirmed, OpenAlex-only Dogfooding showed live search felt slow and fired too eagerly. - Live search now queries OpenAlex only (LIVE_SOURCES) instead of also waiting on Semantic Scholar (~1 req/s) and PubMed; latency drops from ~tens of seconds to ~1s. Batch sync still uses all three. - Lower default timeout to 15s. - Tool description + EEGLAB prompt require local-first: always search the indexed library first, OFFER a live search and wait for confirmation, and announce 'this might take a minute' before running it. Never call the live tool as the first action on a paper question. Closes #312 * docs(tool): align live-search description with announce-then-call framing Address PR review: a tool description can't enforce pre-call sequencing, so phrase the announce guidance as 'when you call it, your message in that turn should first tell the user...' - matters most for communities that rely on the description without extra prompt guidance. * fix(papers): address full PR review on live search silent-failure-hunter: - narrow live-search error handling: APIKeyError/ConfigurationError -> error log, OpenCiteError -> warning, both return []; let programming errors propagate instead of masquerading as 'no results'. - escalate cache-write failure to logger.error(exc_info=True) (a lost cache write is a real degraded state). - tool 'no results' message no longer falsely implies a timeout. comment-analyzer: - correct overstated duration: ~15s cap, not 'a minute' (tool description + EEGLAB prompt say 'a few seconds'). - tighten DEFAULT_SOURCES/LIVE_SOURCES rationale comments. pr-test-analyzer: - add TestSourceConstants (live = OpenAlex-only, strict subset of batch sources); exercise the new sources param + production timeout in the live test. --- src/assistants/eeglab/config.yaml | 14 +++++--- src/knowledge/papers_sync.py | 45 ++++++++++++++++++------ src/tools/knowledge.py | 18 ++++++---- tests/test_knowledge/test_papers_sync.py | 19 +++++++++- 4 files changed, 72 insertions(+), 24 deletions(-) diff --git a/src/assistants/eeglab/config.yaml b/src/assistants/eeglab/config.yaml index 7c67128..8e4f338 100644 --- a/src/assistants/eeglab/config.yaml +++ b/src/assistants/eeglab/config.yaml @@ -146,8 +146,8 @@ system_prompt: | 5. `search_eeglab_faq`: Search mailing list Q&A (archives since 2004) **Research:** - 6. `search_eeglab_papers`: Search already-indexed academic literature about EEGLAB and EEG analysis - 7. `search_eeglab_papers_live`: Live on-demand search of the latest literature (newest first) when the user asks for recent/new papers or the indexed search comes up short + 6. `search_eeglab_papers`: Search our already-indexed academic literature about EEGLAB and EEG analysis (instant - use this first) + 7. `search_eeglab_papers_live`: SLOW live search of the latest external literature; only after the user confirms they want it (see the Papers flow below) ## Tool Usage Guidelines @@ -193,12 +193,16 @@ system_prompt: | **Core EEGLAB papers tracked for citations (DOIs in database):** {paper_dois} - **MANDATORY: Use tools for citation/paper questions:** + **MANDATORY: paper/citation questions use the LOCAL index first:** - "Has anyone cited the EEGLAB paper?" -> CALL `search_eeglab_papers(query="EEGLAB")` - "Papers about ICA in EEGLAB?" -> CALL `search_eeglab_papers(query="ICA EEGLAB")` - "Research on ICLabel?" -> CALL `search_eeglab_papers(query="ICLabel")` - - "What are the latest/newest papers on ICLabel?" -> CALL `search_eeglab_papers_live(query="ICLabel")` - - "Any recent papers using clean_rawdata?" -> CALL `search_eeglab_papers_live(query="clean_rawdata ASR EEG")` + + **Live literature search (slow - ALWAYS ask before running it):** + - For ANY paper question, use `search_eeglab_papers` (our indexed library) FIRST and present those results. + - If the user then wants the *latest/newest* papers, or the indexed results are thin, OFFER a live search rather than running it: e.g. "I can search the latest literature for this - it takes a few seconds. Want me to?" Then STOP and wait for their reply. + - Only after the user confirms (or if they explicitly asked to "search the web" / "latest literature"), first say "OK, let me search through the latest literature - this might take a few seconds." and THEN CALL `search_eeglab_papers_live(query="ICLabel")`. + - NEVER call `search_eeglab_papers_live` as the first action on a paper question. **DO NOT:** - Tell users to "visit GitHub", "check Google Scholar", or "use the API" when you have the data diff --git a/src/knowledge/papers_sync.py b/src/knowledge/papers_sync.py index b981207..280af36 100644 --- a/src/knowledge/papers_sync.py +++ b/src/knowledge/papers_sync.py @@ -21,6 +21,7 @@ from opencite import Config, Paper from opencite.citations import CitationExplorer +from opencite.exceptions import APIKeyError, ConfigurationError, OpenCiteError from opencite.search import SearchOrchestrator from src.knowledge.db import get_connection, update_sync_metadata, upsert_paper @@ -28,12 +29,19 @@ logger = logging.getLogger(__name__) -# Scholarly sources synced by default. opencite also supports arxiv, biorxiv, -# medrxiv, osf, zenodo, figshare, crossref and core; those broader sources are -# reserved for the opt-in live-search feature (issue #308) so batch sync stays -# focused on peer-reviewed literature and matches prior coverage. +# Scholarly sources synced by default (batch sync, where latency does not +# matter). opencite also supports arxiv, biorxiv, medrxiv, osf, zenodo, +# figshare, crossref and core; those cover preprints / grey literature and are +# deliberately omitted so the default batch sync stays focused on peer-reviewed +# work. DEFAULT_SOURCES: tuple[str, ...] = ("openalex", "s2", "pubmed") +# Interactive live search uses OpenAlex only: it is fast, free, comprehensive, +# and supports server-side recency sorting (by publication date), so the chat +# stays responsive. The slower, rate-limited sources (Semantic Scholar at +# ~1 req/s, PubMed) are deliberately left to batch sync. +LIVE_SOURCES: tuple[str, ...] = ("openalex",) + # opencite source name -> OSA `papers.source` label. Kept stable so dedup and # the existing rows in the database (openalex / semanticscholar / pubmed) line # up with newly synced papers. @@ -437,6 +445,7 @@ async def _search_recent( query: str, limit: int, timeout: float, + sources: tuple[str, ...], ) -> list[Paper]: """Live opencite search for the most recent papers, bounded by a timeout. @@ -447,7 +456,7 @@ async def _search_recent( """ async with SearchOrchestrator(config) as searcher: result = await asyncio.wait_for( - searcher.search(query, max_results=limit, sources=DEFAULT_SOURCES, sort="year"), + searcher.search(query, max_results=limit, sources=sources, sort="year"), timeout=timeout, ) return result.papers @@ -464,8 +473,11 @@ def _cache_papers_async(papers: list[Paper], project: str) -> threading.Thread: def _write() -> None: try: _store_papers(papers, project) - except Exception as e: - logger.warning("Failed to cache live search papers for %s: %s", project, e) + except Exception: + # A failed cache write means these papers stay missing from local + # search until the next batch sync - a real degraded state, so log + # loudly (with traceback) even though the daemon thread must not crash. + logger.error("Failed to cache live search papers for %s", project, exc_info=True) thread = threading.Thread(target=_write, name=f"cache-papers-{project}", daemon=True) thread.start() @@ -477,7 +489,8 @@ def search_papers_live( project: str = "hed", limit: int = 5, cache: bool = True, - timeout: float = 20.0, + timeout: float = 15.0, + sources: tuple[str, ...] = LIVE_SOURCES, ) -> list[SearchResult]: """Search the live literature via opencite for the most recent papers. @@ -494,22 +507,32 @@ def search_papers_live( community knowledge DB (in a background thread, never blocking the response) so future local searches find them. timeout: Hard cap (seconds) on the opencite call to keep chat snappy. + sources: opencite sources to query. Defaults to OpenAlex only for speed. Returns: - List of SearchResult, newest first. Empty on timeout/error. + List of SearchResult, newest first. Empty on timeout or a transient / + misconfiguration error (logged); programming errors propagate. """ config = _config_from_env() # Bound each source request just under the overall cap so opencite's # per-source tasks finish cleanly before wait_for would cancel them. config.timeout = max(1.0, timeout - 2.0) try: - papers = _run(_search_recent(config, query, limit, timeout)) + papers = _run(_search_recent(config, query, limit, timeout, sources)) except TimeoutError: logger.warning("opencite live search timed out for '%s' after %.0fs", query, timeout) return [] - except Exception as e: + except (APIKeyError, ConfigurationError) as e: + # Permanent misconfiguration (bad/absent key) - surface loudly; it will + # not fix itself and otherwise looks identical to "no results". + logger.error("opencite live search misconfigured for '%s': %s", query, e) + return [] + except OpenCiteError as e: + # Transient API/network/rate-limit failure - a warning + empty is fine. logger.warning("opencite live search failed for '%s': %s", query, e) return [] + # Any other exception is a programming error: let it propagate rather than + # masquerade as an empty result set. if cache and papers: _cache_papers_async(papers, project) diff --git a/src/tools/knowledge.py b/src/tools/knowledge.py index d387570..9409fbd 100644 --- a/src/tools/knowledge.py +++ b/src/tools/knowledge.py @@ -280,8 +280,8 @@ def search_papers_live_impl(query: str, limit: int = 5) -> str: if not results: return ( - f"No recent papers found online for '{query}' " - "(the live search may have timed out; try the local paper search)." + f"No recent papers found online for '{query}'. " + "Try rephrasing, or use the local paper search." ) lines = ["Most recent papers (live search):\n"] @@ -298,11 +298,15 @@ def search_papers_live_impl(query: str, limit: int = 5) -> str: return "\n".join(lines) description = ( - f"Search the LIVE literature for the most recent papers about {community_name}. " - "**Use this only when the user explicitly asks for recent / latest / new papers, " - "or when the local paper search returns nothing relevant.** " - "It queries external sources on demand (slower than the local search) and returns " - "the newest results first. " + f"Live, on-demand search of the latest external literature about {community_name}, " + "newest first. It is slower than the local search (queries the web on demand; " + "up to ~15 seconds). " + f"Always try the local `search_{community_id}_papers` first. " + "**Only call this tool after the user has explicitly confirmed they want a live " + "literature search** (or explicitly asked to search the web / for the very latest " + "papers). Do NOT call it automatically as a first step. When you call it, your " + "message in that turn should first tell the user you are searching the latest " + "literature and it may take a few seconds. " "**This is for DISCOVERY, not answering** - present results as references for " "further reading; do NOT use paper content to formulate answers." ) diff --git a/tests/test_knowledge/test_papers_sync.py b/tests/test_knowledge/test_papers_sync.py index e6cbde2..b23740c 100644 --- a/tests/test_knowledge/test_papers_sync.py +++ b/tests/test_knowledge/test_papers_sync.py @@ -268,16 +268,33 @@ def test_caches_papers_into_db(self, temp_db: Path): assert count == 1 +class TestSourceConstants: + """Enforce the live-vs-batch source contract (deterministic, no network).""" + + def test_live_sources_is_openalex_only(self): + assert ps.LIVE_SOURCES == ("openalex",) + + def test_default_sources_covers_all_three(self): + assert set(ps.DEFAULT_SOURCES) == {"openalex", "s2", "pubmed"} + + def test_live_sources_is_strict_subset_of_default(self): + # Live search must never query more sources than batch sync. + assert set(ps.LIVE_SOURCES) < set(ps.DEFAULT_SOURCES) + + class TestLivePaperSearch: """Live opencite search (real network).""" def test_live_search_returns_recent(self, temp_db: Path): with patch("src.knowledge.db.get_db_path", return_value=temp_db): + # Pass `sources` explicitly to exercise the parameter threading, and + # use the production default timeout. results = search_papers_live( "EEGLAB EEG independent component analysis", project="test", limit=3, - timeout=40, + timeout=15, + sources=("openalex",), ) # Network-dependent: accept empty on transient failure, but the shape From 71b2a8514d4c4549557592ffdd7fe3518ab401d7 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 5 Jun 2026 09:15:19 +0000 Subject: [PATCH 33/39] Bump version to 0.8.4.dev16 --- src/version.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/version.py b/src/version.py index d3ebbe8..c918c9c 100644 --- a/src/version.py +++ b/src/version.py @@ -1,7 +1,7 @@ """Version information for OSA.""" -__version__ = "0.8.4.dev15" -__version_info__ = (0, 8, 4, "dev15") +__version__ = "0.8.4.dev16" +__version_info__ = (0, 8, 4, "dev16") def get_version() -> str: From e7a849c233de19fa1dcd02fe2836d30b13ee6de4 Mon Sep 17 00:00:00 2001 From: "Seyed (Yahya) Shirazi" Date: Fri, 5 Jun 2026 02:30:30 -0700 Subject: [PATCH 34/39] fix(ci): repair invalid YAML in release.yml (#314) The 'Append widget SRI hash' step put its markdown heredoc at 6-space indentation inside a 'run: |' block whose content base is 10 spaces. That ended the YAML block scalar early and the parser hit '**Standard**' as a YAML alias (ScannerError at line 77), making the whole workflow file invalid. GitHub then emitted a startup_failure run on EVERY push (develop and feature branches), regardless of the tags-only trigger. Rewrite the step to pass version/hash via env vars into a quoted heredoc and build the section as an explicit list of lines, so all content stays within the block scalar. Verified: yaml.safe_load parses, trigger is still push.tags v*, and the rendered notes section is correct. Closes #311 --- .github/workflows/release.yml | 49 ++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 758c515..bc8bc62 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -67,28 +67,35 @@ jobs: content = f.read() print('sha384-' + base64.b64encode(hashlib.sha384(content).digest()).decode()) ") - python3 - < - \`\`\` - - **Versioned + SRI** (for supply-chain-secure environments): - \`\`\`html - - \`\`\` - > Pin to this exact release. Update the tag when upgrading. - """ - pathlib.Path('/tmp/release_notes.md').write_text(notes + section) + + version = os.environ["OSA_VERSION"] + sri = os.environ["OSA_SRI"] + section = [ + "", + "## Widget Embedding", + "", + "**Standard** (always latest):", + "```html", + '', + "```", + "", + "**Versioned + SRI** (for supply-chain-secure environments):", + "```html", + f'", + "```", + "> Pin to this exact release. Update the tag when upgrading.", + "", + ] + path = pathlib.Path("/tmp/release_notes.md") + path.write_text(path.read_text() + "\n".join(section)) PYEOF - name: Create GitHub Release From 72e02284d38f68c255274bf1d6750962162788c0 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 5 Jun 2026 09:30:44 +0000 Subject: [PATCH 35/39] Bump version to 0.8.4.dev17 --- src/version.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/version.py b/src/version.py index c918c9c..19f5a63 100644 --- a/src/version.py +++ b/src/version.py @@ -1,7 +1,7 @@ """Version information for OSA.""" -__version__ = "0.8.4.dev16" -__version_info__ = (0, 8, 4, "dev16") +__version__ = "0.8.4.dev17" +__version_info__ = (0, 8, 4, "dev17") def get_version() -> str: From 50addd25e48bbfae01ced688f03f7fedcb55b274 Mon Sep 17 00:00:00 2001 From: "Seyed (Yahya) Shirazi" Date: Fri, 5 Jun 2026 12:46:30 -0700 Subject: [PATCH 36/39] chore(deps): bump opencite to >=0.5.3 (#315) opencite 0.5.3 ships the search-task cancellation fix (neuromechanist/opencite#41): search() now cancels and drains pending per-source tasks in a finally block, so a wait_for timeout no longer orphans tasks that later touch closed HTTP clients. Bump the floor to >=0.5.3 to require the fix and refresh uv.lock (0.5.2 -> 0.5.3). No other packages change. --- pyproject.toml | 2 +- uv.lock | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 83f9a74..b45550c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -66,7 +66,7 @@ server = [ "markdownify>=1.1.0", # Scheduling "apscheduler>=3.10.0,<4.0.0", - "opencite>=0.5.2", + "opencite>=0.5.3", ] observability = [ diff --git a/uv.lock b/uv.lock index 6995cbd..484cc1d 100644 --- a/uv.lock +++ b/uv.lock @@ -2114,8 +2114,8 @@ requires-dist = [ { name = "markdownify", marker = "extra == 'dev'", specifier = ">=1.1.0" }, { name = "markdownify", marker = "extra == 'server'", specifier = ">=1.1.0" }, { name = "mypy", marker = "extra == 'dev'", specifier = ">=1.19.0" }, - { name = "opencite", marker = "extra == 'dev'", specifier = ">=0.5.2" }, - { name = "opencite", marker = "extra == 'server'", specifier = ">=0.5.2" }, + { name = "opencite", marker = "extra == 'dev'", specifier = ">=0.5.3" }, + { name = "opencite", marker = "extra == 'server'", specifier = ">=0.5.3" }, { name = "platformdirs", specifier = ">=4.5.0" }, { name = "pre-commit", marker = "extra == 'dev'", specifier = ">=4.5.0" }, { name = "psycopg", extras = ["binary"], marker = "extra == 'dev'", specifier = ">=3.3.0" }, @@ -2163,15 +2163,15 @@ wheels = [ [[package]] name = "opencite" -version = "0.5.2" +version = "0.5.3" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "httpx" }, { name = "pyalex" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/7a/18/313a129d8cee89784c409ec19049f09adf6ba515792747b9dd65dbbd9ddb/opencite-0.5.2.tar.gz", hash = "sha256:d7a63482e4d1a0372fd2d01cc246e3d10b74d50393ae153afe53029b490e7d7e", size = 142084, upload-time = "2026-05-21T19:54:15.971Z" } +sdist = { url = "https://files.pythonhosted.org/packages/fd/47/2cef932d68ca50361afb888ac18592fcfdb8ea0f4d18df9f0af61b0765b9/opencite-0.5.3.tar.gz", hash = "sha256:c1e19d8253615644870be95a4d7bbb09540a33b1e241c43d451348aa3ef48df5", size = 157917, upload-time = "2026-06-05T18:59:56.706Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/0e/b8/d00880ff96e9d3808c6b65e6347b7c4f9fdc8b72213a5dd4cd147224207e/opencite-0.5.2-py3-none-any.whl", hash = "sha256:d4ffb6b553c94df454dbc1de4eed55fdc2ea7604099fea0e3fdbc5a5ea654ce1", size = 96732, upload-time = "2026-05-21T19:54:14.574Z" }, + { url = "https://files.pythonhosted.org/packages/9f/f1/7925aa055263bc62e3b4cded285e7992c167a85716df12cf456372701c2d/opencite-0.5.3-py3-none-any.whl", hash = "sha256:e8607ebff6eeda30b86362c3031307773c99925df9c429353c802b04f50c66ec", size = 97003, upload-time = "2026-06-05T18:59:55.284Z" }, ] [[package]] From b6a22d7bbdadd24afbc21f4df4c6e36d2fcf44e9 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 5 Jun 2026 19:46:40 +0000 Subject: [PATCH 37/39] Bump version to 0.8.4.dev18 --- src/version.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/version.py b/src/version.py index 19f5a63..d3f72ad 100644 --- a/src/version.py +++ b/src/version.py @@ -1,7 +1,7 @@ """Version information for OSA.""" -__version__ = "0.8.4.dev17" -__version_info__ = (0, 8, 4, "dev17") +__version__ = "0.8.4.dev18" +__version_info__ = (0, 8, 4, "dev18") def get_version() -> str: From 6c873f2c2e2c37fb152de9d4a728d9dc7eb9717b Mon Sep 17 00:00:00 2001 From: "Seyed (Yahya) Shirazi" Date: Mon, 8 Jun 2026 11:02:42 -0700 Subject: [PATCH 38/39] fix: pre-release hardening for 0.8.4 (#317) * feat(papers): make live_search opt-in (default off), enable for EEGLAB Live web paper search added external-API latency to every community with a citations block. Default to False so communities opt in explicitly; enable it for EEGLAB, whose prompt already tells the agent to ask before running it. * fix(papers): harden opencite sync error handling Narrow the broad except in the batch search/citation loops to (OpenCiteError, TimeoutError) for expected API failures; log any other exception with a full traceback (logger.exception) so a bug no longer masquerades as a routine 'no results'. Isolate per-query/per-DOI _store_papers so one DB failure cannot abort the batch or desync metadata. Add type hints to _run. * chore(deps): drop direct pyalex dependency pyalex is no longer imported anywhere; opencite pulls it in transitively. Remove the redundant direct declaration and refresh the stale OpenAlex comment. * fix(feedback): return 422 not 500 on FeedbackEntry invariant drift FeedbackRequest and FeedbackEntry validate the same invariants independently; if they ever drift, guard the construction so a bad shape returns a clean 422 instead of an unhandled 500. Add a whitespace-only general-comment rejection test. * fix(widget): timeout and double-submit guard on general feedback postFeedback now aborts after 10s instead of hanging indefinitely, and the general-feedback Send button is disabled while the POST is in flight so a double-click cannot store two rows. * style: ruff-format widget-sri.py --- frontend/osa-chat-widget.js | 26 +++++++--- pyproject.toml | 1 - scripts/widget-sri.py | 2 +- src/api/routers/feedback.py | 29 ++++++----- src/assistants/eeglab/config.yaml | 1 + src/cli/sync.py | 2 +- src/core/config/community.py | 8 +++- src/knowledge/papers_sync.py | 48 ++++++++++++++----- tests/test_api/test_feedback_endpoint.py | 9 ++++ tests/test_core/test_config/test_community.py | 5 ++ uv.lock | 9 ++-- 11 files changed, 100 insertions(+), 40 deletions(-) diff --git a/frontend/osa-chat-widget.js b/frontend/osa-chat-widget.js index 7065e82..a76045a 100644 --- a/frontend/osa-chat-widget.js +++ b/frontend/osa-chat-widget.js @@ -1938,6 +1938,7 @@ method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ community_id: CONFIG.communityId, ...payload }), + signal: AbortSignal.timeout(10000), }); if (!response.ok) { console.warn('[OSA] Feedback submission returned', response.status); @@ -2069,12 +2070,25 @@ return; } - const ok = await postFeedback({ - feedback_type: 'general', - comment, - session_id: sessionId || null, - page_url: (typeof window !== 'undefined' && window.location) ? window.location.href : null, - }); + // Guard against a double-click submitting the comment twice while the POST + // is in flight (each would store a separate row). + const sendBtn = container.querySelector('.osa-feedback-send-btn'); + if (sendBtn) { + if (sendBtn.disabled) return; + sendBtn.disabled = true; + } + + let ok = false; + try { + ok = await postFeedback({ + feedback_type: 'general', + comment, + session_id: sessionId || null, + page_url: (typeof window !== 'undefined' && window.location) ? window.location.href : null, + }); + } finally { + if (sendBtn) sendBtn.disabled = false; + } if (ok) { const thanks = container.querySelector('.osa-feedback-modal-thanks'); diff --git a/pyproject.toml b/pyproject.toml index b45550c..410b4af 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -56,7 +56,6 @@ server = [ "langchain-litellm>=0.2.0", # External APIs "pygithub>=2.8.0", - "pyalex>=0.19", # Database "psycopg[binary]>=3.3.0", # Utilities diff --git a/scripts/widget-sri.py b/scripts/widget-sri.py index 29b913c..274a7e3 100644 --- a/scripts/widget-sri.py +++ b/scripts/widget-sri.py @@ -46,7 +46,7 @@ def main() -> None: hash_value = sri(content) print(f"\nSRI hash ({label}):") - print(f" integrity=\"{hash_value}\"") + print(f' integrity="{hash_value}"') if tag: print(f"\nVersioned embed snippet for {tag}:") diff --git a/src/api/routers/feedback.py b/src/api/routers/feedback.py index 392f4e0..b4a0629 100644 --- a/src/api/routers/feedback.py +++ b/src/api/routers/feedback.py @@ -125,18 +125,23 @@ async def submit_feedback( body.community_id, ) - entry = FeedbackEntry( - feedback_id=str(uuid.uuid4()), - timestamp=now_iso(), - community_id=body.community_id, - feedback_type=body.feedback_type, - sentiment=body.sentiment, - request_id=body.request_id, - session_id=body.session_id, - message_index=body.message_index, - comment=body.comment, - page_url=body.page_url, - ) + try: + entry = FeedbackEntry( + feedback_id=str(uuid.uuid4()), + timestamp=now_iso(), + community_id=body.community_id, + feedback_type=body.feedback_type, + sentiment=body.sentiment, + request_id=body.request_id, + session_id=body.session_id, + message_index=body.message_index, + comment=body.comment, + page_url=body.page_url, + ) + except ValueError as e: + # FeedbackRequest already validates these invariants; this guards against + # the two validators drifting apart so a bad shape returns 422, not 500. + raise HTTPException(status_code=422, detail=str(e)) from e # write_feedback is best-effort: it logs and swallows storage errors (and # escalates after repeated failures) rather than failing the user's request, diff --git a/src/assistants/eeglab/config.yaml b/src/assistants/eeglab/config.yaml index 8e4f338..a51c072 100644 --- a/src/assistants/eeglab/config.yaml +++ b/src/assistants/eeglab/config.yaml @@ -414,6 +414,7 @@ github: # Paper/citation search configuration citations: + live_search: true # prompt already instructs the agent to ask before running it queries: - EEGLAB tutorial - EEGLAB plugin diff --git a/src/cli/sync.py b/src/cli/sync.py index e149686..ce4503a 100644 --- a/src/cli/sync.py +++ b/src/cli/sync.py @@ -190,7 +190,7 @@ def _safe_load_config() -> tuple[str | None, str | None]: if not pubmed_key: pubmed_key = os.environ.get("PUBMED_API_KEY") - # Configure OpenAlex from env vars (pyalex uses global config) + # Configure OpenAlex credentials from env vars (merged into the opencite Config) openalex_key = os.environ.get("OPENALEX_API_KEY") openalex_email = os.environ.get("OPENALEX_EMAIL") configure_openalex(api_key=openalex_key, email=openalex_email) diff --git a/src/core/config/community.py b/src/core/config/community.py index c7bce15..75d01b4 100644 --- a/src/core/config/community.py +++ b/src/core/config/community.py @@ -236,8 +236,12 @@ class CitationConfig(BaseModel): dois: list[str] = Field(default_factory=list) """Core paper DOIs to track citations for (format: '10.xxxx/yyyy').""" - live_search: bool = Field(default=True) - """Expose an on-demand live paper search tool (opencite) for recent literature.""" + live_search: bool = Field(default=False) + """Expose an on-demand live paper search tool (opencite) for recent literature. + + Off by default: the tool adds external-API latency to a turn and queries + OpenAlex anonymously. Communities opt in explicitly, and their prompt should + tell the agent to ask the user before running it.""" @field_validator("queries") @classmethod diff --git a/src/knowledge/papers_sync.py b/src/knowledge/papers_sync.py index 280af36..a83806b 100644 --- a/src/knowledge/papers_sync.py +++ b/src/knowledge/papers_sync.py @@ -16,8 +16,9 @@ import logging import os import threading -from collections.abc import Iterable +from collections.abc import Coroutine, Iterable from concurrent.futures import ThreadPoolExecutor +from typing import Any, TypeVar from opencite import Config, Paper from opencite.citations import CitationExplorer @@ -198,7 +199,10 @@ def _store_papers( return counts -def _run(coro): +_T = TypeVar("_T") + + +def _run(coro: Coroutine[Any, Any, _T]) -> _T: """Execute an async coroutine from synchronous code. OSA's sync callers (CLI command, scheduler thread) have no running event @@ -232,9 +236,15 @@ async def _search_queries( try: result = await searcher.search(query, max_results=max_results, sources=sources) out.append((query, result.papers)) - except Exception as e: + except (OpenCiteError, TimeoutError) as e: logger.warning("opencite search error for '%s': %s", query, e) out.append((query, [])) + except Exception: + # Unexpected (likely a bug, not an API failure): keep the batch + # going but log loudly with a traceback so it is not mistaken + # for a routine "no results" outcome. + logger.exception("unexpected error searching '%s'", query) + out.append((query, [])) return out @@ -250,9 +260,12 @@ async def _citing_for_dois( try: result = await explorer.citing_papers(doi, max_results=max_results) out.append((doi, result.papers)) - except Exception as e: + except (OpenCiteError, TimeoutError) as e: logger.warning("opencite citation error for DOI %s: %s", doi, e) out.append((doi, [])) + except Exception: + logger.exception("unexpected error fetching citations for DOI %s", doi) + out.append((doi, [])) return out @@ -357,10 +370,15 @@ def sync_all_papers( return results for query, papers in searched: - counts = _store_papers(papers, project) - for source, n in counts.items(): - results[source] = results.get(source, 0) + n - update_sync_metadata("papers", f"opencite:{query}", sum(counts.values()), project) + try: + counts = _store_papers(papers, project) + for source, n in counts.items(): + results[source] = results.get(source, 0) + n + update_sync_metadata("papers", f"opencite:{query}", sum(counts.values()), project) + except Exception: + # Isolate per-query: a DB failure on one query must not abort the + # whole batch or leave sync metadata inconsistent for the others. + logger.exception("failed to store papers for '%s' (%s)", query, project) total = sum(results.values()) logger.info("Total papers synced for %s: %d", project, total) @@ -401,11 +419,15 @@ def sync_citing_papers( total = 0 for doi, papers in cited: - counts = _store_papers(papers, project) - count = sum(counts.values()) - update_sync_metadata("papers", f"citing_{doi}", count, project) - logger.info("Synced %d papers citing %s", count, doi) - total += count + try: + counts = _store_papers(papers, project) + count = sum(counts.values()) + update_sync_metadata("papers", f"citing_{doi}", count, project) + logger.info("Synced %d papers citing %s", count, doi) + total += count + except Exception: + # Isolate per-DOI so one DB failure does not abort the batch. + logger.exception("failed to store citing papers for %s (%s)", doi, project) return total diff --git a/tests/test_api/test_feedback_endpoint.py b/tests/test_api/test_feedback_endpoint.py index b307cf8..38540b3 100644 --- a/tests/test_api/test_feedback_endpoint.py +++ b/tests/test_api/test_feedback_endpoint.py @@ -116,6 +116,15 @@ def test_general_without_comment_rejected(self, client): ) assert resp.status_code == 422 + def test_general_whitespace_comment_rejected(self, client): + # _normalize collapses a whitespace-only comment to None, and _check_shape + # then rejects general feedback that carries no real comment. + resp = client.post( + "/feedback", + json={"community_id": "hed", "feedback_type": "general", "comment": " "}, + ) + assert resp.status_code == 422 + def test_oversized_comment_rejected(self, client): resp = client.post( "/feedback", diff --git a/tests/test_core/test_config/test_community.py b/tests/test_core/test_config/test_community.py index 1e4ae5c..ab5f8f8 100644 --- a/tests/test_core/test_config/test_community.py +++ b/tests/test_core/test_config/test_community.py @@ -150,6 +150,11 @@ def test_empty_defaults(self) -> None: assert config.queries == [] assert config.dois == [] + def test_live_search_off_by_default(self) -> None: + """Live search is opt-in: a community must enable it explicitly.""" + assert CitationConfig().live_search is False + assert CitationConfig(live_search=True).live_search is True + def test_validates_doi_format(self) -> None: """Should validate DOI format.""" with pytest.raises(ValidationError, match="Invalid DOI format"): diff --git a/uv.lock b/uv.lock index 484cc1d..8824ba3 100644 --- a/uv.lock +++ b/uv.lock @@ -811,6 +811,7 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/1f/cb/48e964c452ca2b92175a9b2dca037a553036cb053ba69e284650ce755f13/greenlet-3.3.0-cp311-cp311-macosx_11_0_universal2.whl", hash = "sha256:e29f3018580e8412d6aaf5641bb7745d38c85228dacf51a73bd4e26ddf2a6a8e", size = 274908, upload-time = "2025-12-04T14:23:26.435Z" }, { url = "https://files.pythonhosted.org/packages/28/da/38d7bff4d0277b594ec557f479d65272a893f1f2a716cad91efeb8680953/greenlet-3.3.0-cp311-cp311-manylinux_2_24_aarch64.manylinux_2_28_aarch64.whl", hash = "sha256:a687205fb22794e838f947e2194c0566d3812966b41c78709554aa883183fb62", size = 577113, upload-time = "2025-12-04T14:50:05.493Z" }, { url = "https://files.pythonhosted.org/packages/3c/f2/89c5eb0faddc3ff014f1c04467d67dee0d1d334ab81fadbf3744847f8a8a/greenlet-3.3.0-cp311-cp311-manylinux_2_24_ppc64le.manylinux_2_28_ppc64le.whl", hash = "sha256:4243050a88ba61842186cb9e63c7dfa677ec146160b0efd73b855a3d9c7fcf32", size = 590338, upload-time = "2025-12-04T14:57:41.136Z" }, + { url = "https://files.pythonhosted.org/packages/80/d7/db0a5085035d05134f8c089643da2b44cc9b80647c39e93129c5ef170d8f/greenlet-3.3.0-cp311-cp311-manylinux_2_24_s390x.manylinux_2_28_s390x.whl", hash = "sha256:670d0f94cd302d81796e37299bcd04b95d62403883b24225c6b5271466612f45", size = 601098, upload-time = "2025-12-04T15:07:11.898Z" }, { url = "https://files.pythonhosted.org/packages/dc/a6/e959a127b630a58e23529972dbc868c107f9d583b5a9f878fb858c46bc1a/greenlet-3.3.0-cp311-cp311-manylinux_2_24_x86_64.manylinux_2_28_x86_64.whl", hash = "sha256:6cb3a8ec3db4a3b0eb8a3c25436c2d49e3505821802074969db017b87bc6a948", size = 590206, upload-time = "2025-12-04T14:26:01.254Z" }, { url = "https://files.pythonhosted.org/packages/48/60/29035719feb91798693023608447283b266b12efc576ed013dd9442364bb/greenlet-3.3.0-cp311-cp311-musllinux_1_2_aarch64.whl", hash = "sha256:2de5a0b09eab81fc6a382791b995b1ccf2b172a9fec934747a7a23d2ff291794", size = 1550668, upload-time = "2025-12-04T15:04:22.439Z" }, { url = "https://files.pythonhosted.org/packages/0a/5f/783a23754b691bfa86bd72c3033aa107490deac9b2ef190837b860996c9f/greenlet-3.3.0-cp311-cp311-musllinux_1_2_x86_64.whl", hash = "sha256:4449a736606bd30f27f8e1ff4678ee193bc47f6ca810d705981cfffd6ce0d8c5", size = 1615483, upload-time = "2025-12-04T14:27:28.083Z" }, @@ -818,6 +819,7 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/f8/0a/a3871375c7b9727edaeeea994bfff7c63ff7804c9829c19309ba2e058807/greenlet-3.3.0-cp312-cp312-macosx_11_0_universal2.whl", hash = "sha256:b01548f6e0b9e9784a2c99c5651e5dc89ffcbe870bc5fb2e5ef864e9cc6b5dcb", size = 276379, upload-time = "2025-12-04T14:23:30.498Z" }, { url = "https://files.pythonhosted.org/packages/43/ab/7ebfe34dce8b87be0d11dae91acbf76f7b8246bf9d6b319c741f99fa59c6/greenlet-3.3.0-cp312-cp312-manylinux_2_24_aarch64.manylinux_2_28_aarch64.whl", hash = "sha256:349345b770dc88f81506c6861d22a6ccd422207829d2c854ae2af8025af303e3", size = 597294, upload-time = "2025-12-04T14:50:06.847Z" }, { url = "https://files.pythonhosted.org/packages/a4/39/f1c8da50024feecd0793dbd5e08f526809b8ab5609224a2da40aad3a7641/greenlet-3.3.0-cp312-cp312-manylinux_2_24_ppc64le.manylinux_2_28_ppc64le.whl", hash = "sha256:e8e18ed6995e9e2c0b4ed264d2cf89260ab3ac7e13555b8032b25a74c6d18655", size = 607742, upload-time = "2025-12-04T14:57:42.349Z" }, + { url = "https://files.pythonhosted.org/packages/77/cb/43692bcd5f7a0da6ec0ec6d58ee7cddb606d055ce94a62ac9b1aa481e969/greenlet-3.3.0-cp312-cp312-manylinux_2_24_s390x.manylinux_2_28_s390x.whl", hash = "sha256:c024b1e5696626890038e34f76140ed1daf858e37496d33f2af57f06189e70d7", size = 622297, upload-time = "2025-12-04T15:07:13.552Z" }, { url = "https://files.pythonhosted.org/packages/75/b0/6bde0b1011a60782108c01de5913c588cf51a839174538d266de15e4bf4d/greenlet-3.3.0-cp312-cp312-manylinux_2_24_x86_64.manylinux_2_28_x86_64.whl", hash = "sha256:047ab3df20ede6a57c35c14bf5200fcf04039d50f908270d3f9a7a82064f543b", size = 609885, upload-time = "2025-12-04T14:26:02.368Z" }, { url = "https://files.pythonhosted.org/packages/49/0e/49b46ac39f931f59f987b7cd9f34bfec8ef81d2a1e6e00682f55be5de9f4/greenlet-3.3.0-cp312-cp312-musllinux_1_2_aarch64.whl", hash = "sha256:2d9ad37fc657b1102ec880e637cccf20191581f75c64087a549e66c57e1ceb53", size = 1567424, upload-time = "2025-12-04T15:04:23.757Z" }, { url = "https://files.pythonhosted.org/packages/05/f5/49a9ac2dff7f10091935def9165c90236d8f175afb27cbed38fb1d61ab6b/greenlet-3.3.0-cp312-cp312-musllinux_1_2_x86_64.whl", hash = "sha256:83cd0e36932e0e7f36a64b732a6f60c2fc2df28c351bae79fbaf4f8092fe7614", size = 1636017, upload-time = "2025-12-04T14:27:29.688Z" }, @@ -825,6 +827,7 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/02/2f/28592176381b9ab2cafa12829ba7b472d177f3acc35d8fbcf3673d966fff/greenlet-3.3.0-cp313-cp313-macosx_11_0_universal2.whl", hash = "sha256:a1e41a81c7e2825822f4e068c48cb2196002362619e2d70b148f20a831c00739", size = 275140, upload-time = "2025-12-04T14:23:01.282Z" }, { url = "https://files.pythonhosted.org/packages/2c/80/fbe937bf81e9fca98c981fe499e59a3f45df2a04da0baa5c2be0dca0d329/greenlet-3.3.0-cp313-cp313-manylinux_2_24_aarch64.manylinux_2_28_aarch64.whl", hash = "sha256:9f515a47d02da4d30caaa85b69474cec77b7929b2e936ff7fb853d42f4bf8808", size = 599219, upload-time = "2025-12-04T14:50:08.309Z" }, { url = "https://files.pythonhosted.org/packages/c2/ff/7c985128f0514271b8268476af89aee6866df5eec04ac17dcfbc676213df/greenlet-3.3.0-cp313-cp313-manylinux_2_24_ppc64le.manylinux_2_28_ppc64le.whl", hash = "sha256:7d2d9fd66bfadf230b385fdc90426fcd6eb64db54b40c495b72ac0feb5766c54", size = 610211, upload-time = "2025-12-04T14:57:43.968Z" }, + { url = "https://files.pythonhosted.org/packages/79/07/c47a82d881319ec18a4510bb30463ed6891f2ad2c1901ed5ec23d3de351f/greenlet-3.3.0-cp313-cp313-manylinux_2_24_s390x.manylinux_2_28_s390x.whl", hash = "sha256:30a6e28487a790417d036088b3bcb3f3ac7d8babaa7d0139edbaddebf3af9492", size = 624311, upload-time = "2025-12-04T15:07:14.697Z" }, { url = "https://files.pythonhosted.org/packages/fd/8e/424b8c6e78bd9837d14ff7df01a9829fc883ba2ab4ea787d4f848435f23f/greenlet-3.3.0-cp313-cp313-manylinux_2_24_x86_64.manylinux_2_28_x86_64.whl", hash = "sha256:087ea5e004437321508a8d6f20efc4cfec5e3c30118e1417ea96ed1d93950527", size = 612833, upload-time = "2025-12-04T14:26:03.669Z" }, { url = "https://files.pythonhosted.org/packages/b5/ba/56699ff9b7c76ca12f1cdc27a886d0f81f2189c3455ff9f65246780f713d/greenlet-3.3.0-cp313-cp313-musllinux_1_2_aarch64.whl", hash = "sha256:ab97cf74045343f6c60a39913fa59710e4bd26a536ce7ab2397adf8b27e67c39", size = 1567256, upload-time = "2025-12-04T15:04:25.276Z" }, { url = "https://files.pythonhosted.org/packages/1e/37/f31136132967982d698c71a281a8901daf1a8fbab935dce7c0cf15f942cc/greenlet-3.3.0-cp313-cp313-musllinux_1_2_x86_64.whl", hash = "sha256:5375d2e23184629112ca1ea89a53389dddbffcf417dad40125713d88eb5f96e8", size = 1636483, upload-time = "2025-12-04T14:27:30.804Z" }, @@ -832,6 +835,7 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/d7/7c/f0a6d0ede2c7bf092d00bc83ad5bafb7e6ec9b4aab2fbdfa6f134dc73327/greenlet-3.3.0-cp314-cp314-macosx_11_0_universal2.whl", hash = "sha256:60c2ef0f578afb3c8d92ea07ad327f9a062547137afe91f38408f08aacab667f", size = 275671, upload-time = "2025-12-04T14:23:05.267Z" }, { url = "https://files.pythonhosted.org/packages/44/06/dac639ae1a50f5969d82d2e3dd9767d30d6dbdbab0e1a54010c8fe90263c/greenlet-3.3.0-cp314-cp314-manylinux_2_24_aarch64.manylinux_2_28_aarch64.whl", hash = "sha256:0a5d554d0712ba1de0a6c94c640f7aeba3f85b3a6e1f2899c11c2c0428da9365", size = 646360, upload-time = "2025-12-04T14:50:10.026Z" }, { url = "https://files.pythonhosted.org/packages/e0/94/0fb76fe6c5369fba9bf98529ada6f4c3a1adf19e406a47332245ef0eb357/greenlet-3.3.0-cp314-cp314-manylinux_2_24_ppc64le.manylinux_2_28_ppc64le.whl", hash = "sha256:3a898b1e9c5f7307ebbde4102908e6cbfcb9ea16284a3abe15cab996bee8b9b3", size = 658160, upload-time = "2025-12-04T14:57:45.41Z" }, + { url = "https://files.pythonhosted.org/packages/93/79/d2c70cae6e823fac36c3bbc9077962105052b7ef81db2f01ec3b9bf17e2b/greenlet-3.3.0-cp314-cp314-manylinux_2_24_s390x.manylinux_2_28_s390x.whl", hash = "sha256:dcd2bdbd444ff340e8d6bdf54d2f206ccddbb3ccfdcd3c25bf4afaa7b8f0cf45", size = 671388, upload-time = "2025-12-04T15:07:15.789Z" }, { url = "https://files.pythonhosted.org/packages/b8/14/bab308fc2c1b5228c3224ec2bf928ce2e4d21d8046c161e44a2012b5203e/greenlet-3.3.0-cp314-cp314-manylinux_2_24_x86_64.manylinux_2_28_x86_64.whl", hash = "sha256:5773edda4dc00e173820722711d043799d3adb4f01731f40619e07ea2750b955", size = 660166, upload-time = "2025-12-04T14:26:05.099Z" }, { url = "https://files.pythonhosted.org/packages/4b/d2/91465d39164eaa0085177f61983d80ffe746c5a1860f009811d498e7259c/greenlet-3.3.0-cp314-cp314-musllinux_1_2_aarch64.whl", hash = "sha256:ac0549373982b36d5fd5d30beb8a7a33ee541ff98d2b502714a09f1169f31b55", size = 1615193, upload-time = "2025-12-04T15:04:27.041Z" }, { url = "https://files.pythonhosted.org/packages/42/1b/83d110a37044b92423084d52d5d5a3b3a73cafb51b547e6d7366ff62eff1/greenlet-3.3.0-cp314-cp314-musllinux_1_2_x86_64.whl", hash = "sha256:d198d2d977460358c3b3a4dc844f875d1adb33817f0613f663a656f463764ccc", size = 1683653, upload-time = "2025-12-04T14:27:32.366Z" }, @@ -839,6 +843,7 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/a0/66/bd6317bc5932accf351fc19f177ffba53712a202f9df10587da8df257c7e/greenlet-3.3.0-cp314-cp314t-macosx_11_0_universal2.whl", hash = "sha256:d6ed6f85fae6cdfdb9ce04c9bf7a08d666cfcfb914e7d006f44f840b46741931", size = 282638, upload-time = "2025-12-04T14:25:20.941Z" }, { url = "https://files.pythonhosted.org/packages/30/cf/cc81cb030b40e738d6e69502ccbd0dd1bced0588e958f9e757945de24404/greenlet-3.3.0-cp314-cp314t-manylinux_2_24_aarch64.manylinux_2_28_aarch64.whl", hash = "sha256:d9125050fcf24554e69c4cacb086b87b3b55dc395a8b3ebe6487b045b2614388", size = 651145, upload-time = "2025-12-04T14:50:11.039Z" }, { url = "https://files.pythonhosted.org/packages/9c/ea/1020037b5ecfe95ca7df8d8549959baceb8186031da83d5ecceff8b08cd2/greenlet-3.3.0-cp314-cp314t-manylinux_2_24_ppc64le.manylinux_2_28_ppc64le.whl", hash = "sha256:87e63ccfa13c0a0f6234ed0add552af24cc67dd886731f2261e46e241608bee3", size = 654236, upload-time = "2025-12-04T14:57:47.007Z" }, + { url = "https://files.pythonhosted.org/packages/69/cc/1e4bae2e45ca2fa55299f4e85854606a78ecc37fead20d69322f96000504/greenlet-3.3.0-cp314-cp314t-manylinux_2_24_s390x.manylinux_2_28_s390x.whl", hash = "sha256:2662433acbca297c9153a4023fe2161c8dcfdcc91f10433171cf7e7d94ba2221", size = 662506, upload-time = "2025-12-04T15:07:16.906Z" }, { url = "https://files.pythonhosted.org/packages/57/b9/f8025d71a6085c441a7eaff0fd928bbb275a6633773667023d19179fe815/greenlet-3.3.0-cp314-cp314t-manylinux_2_24_x86_64.manylinux_2_28_x86_64.whl", hash = "sha256:3c6e9b9c1527a78520357de498b0e709fb9e2f49c3a513afd5a249007261911b", size = 653783, upload-time = "2025-12-04T14:26:06.225Z" }, { url = "https://files.pythonhosted.org/packages/f6/c7/876a8c7a7485d5d6b5c6821201d542ef28be645aa024cfe1145b35c120c1/greenlet-3.3.0-cp314-cp314t-musllinux_1_2_aarch64.whl", hash = "sha256:286d093f95ec98fdd92fcb955003b8a3d054b4e2cab3e2707a5039e7b50520fd", size = 1614857, upload-time = "2025-12-04T15:04:28.484Z" }, { url = "https://files.pythonhosted.org/packages/4f/dc/041be1dff9f23dac5f48a43323cd0789cb798342011c19a248d9c9335536/greenlet-3.3.0-cp314-cp314t-musllinux_1_2_x86_64.whl", hash = "sha256:6c10513330af5b8ae16f023e8ddbfb486ab355d04467c4679c5cfe4659975dd9", size = 1676034, upload-time = "2025-12-04T14:27:33.531Z" }, @@ -2043,7 +2048,6 @@ dev = [ { name = "opencite" }, { name = "pre-commit" }, { name = "psycopg", extra = ["binary"] }, - { name = "pyalex" }, { name = "pydantic-settings" }, { name = "pygithub" }, { name = "pytest" }, @@ -2074,7 +2078,6 @@ server = [ { name = "markdownify" }, { name = "opencite" }, { name = "psycopg", extra = ["binary"] }, - { name = "pyalex" }, { name = "pydantic-settings" }, { name = "pygithub" }, { name = "python-dotenv" }, @@ -2120,8 +2123,6 @@ requires-dist = [ { name = "pre-commit", marker = "extra == 'dev'", specifier = ">=4.5.0" }, { name = "psycopg", extras = ["binary"], marker = "extra == 'dev'", specifier = ">=3.3.0" }, { name = "psycopg", extras = ["binary"], marker = "extra == 'server'", specifier = ">=3.3.0" }, - { name = "pyalex", marker = "extra == 'dev'", specifier = ">=0.19" }, - { name = "pyalex", marker = "extra == 'server'", specifier = ">=0.19" }, { name = "pydantic", specifier = ">=2.12.0" }, { name = "pydantic-settings", marker = "extra == 'dev'", specifier = ">=2.12.0" }, { name = "pydantic-settings", marker = "extra == 'server'", specifier = ">=2.12.0" }, From deddd44510bfa5a0bd1ca6dbeaa1e515d9bed257 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Mon, 8 Jun 2026 18:02:56 +0000 Subject: [PATCH 39/39] Bump version to 0.8.4.dev19 --- src/version.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/version.py b/src/version.py index d3f72ad..20b7cc8 100644 --- a/src/version.py +++ b/src/version.py @@ -1,7 +1,7 @@ """Version information for OSA.""" -__version__ = "0.8.4.dev18" -__version_info__ = (0, 8, 4, "dev18") +__version__ = "0.8.4.dev19" +__version_info__ = (0, 8, 4, "dev19") def get_version() -> str: