Skip to content

Enforce mandatory HED tag validation before display#211

Merged
neuromechanist merged 2 commits intodevelopfrom
fix/issue-210-mandatory-hed-validation
Feb 18, 2026
Merged

Enforce mandatory HED tag validation before display#211
neuromechanist merged 2 commits intodevelopfrom
fix/issue-210-mandatory-hed-validation

Conversation

@neuromechanist
Copy link
Member

Summary

  • Strengthen system prompt to make HED tag validation mandatory (not "when in doubt")
  • Make suggest_hed_tags return a visible error when CLI is unavailable instead of silent empty lists
  • Remove "use docs instead" escape hatch from validate_hed_string error messages, replacing with explicit instruction to not present unvalidated tags

Fixes #210

Test plan

  • All existing HED tests pass (uv run pytest tests/ -v -k "hed")
  • Community/config tests pass (496 passed)
  • Ruff lint/format passes
  • Deploy to dev and test with the original query: "How can I annotate muscle artifacts for EEG data?"
  • Verify the assistant calls validate_hed_string before presenting tags
  • Verify that when validation fails, the assistant tells the user instead of hallucinating

@neuromechanist
Copy link
Member Author

PR Review Summary

Ran code-reviewer, silent-failure-hunter, comment-analyzer, and code-simplifier.

Issues Found and Fixed

Critical (2) - Fixed in d57b1ca:

  1. suggest_hed_tags CLI execution errors (non-zero exit, timeout, bad JSON, generic exception) returned silent empty lists without "error" key - same pattern this PR aims to fix. Now all failure paths return visible errors.
  2. validate_hed_string docstring still had old "use known-good example from docs" escape hatch. Updated to match new policy.

Important (2) - Fixed in d57b1ca:

  1. validate_hed_string leaked raw exception text {e} in return dict. Sanitized to generic messages; full details still logged server-side.
  2. Line 88 "never show the validation process" conflicted with "tell user when unavailable". Clarified: hide successful validation, be transparent about failures.

Suggestions (noted, not addressed):

  • Example HED string in prompt (Sensory-event, Visual-presentation, Red) should be verified against HED 8.4.0 schema
  • Section heading "Using Tools (MANDATORY)" slightly misleading since doc retrieval bullets are discretionary
  • Validation mandate repeated in 5 places with slightly different wording

Strengths

  • "What counts as needing validation" section explicitly closes LLM loopholes
  • Error messages use direct imperative language appropriate for LLM agents
  • Step 5 with literal example of what to say is effective for instruction-following

All review issues addressed. Tests pass (134 passed, 4 pre-existing async failures). Lint clean.

@neuromechanist neuromechanist linked an issue Feb 18, 2026 that may be closed by this pull request
@neuromechanist neuromechanist merged commit 2381d79 into develop Feb 18, 2026
8 checks passed
@neuromechanist neuromechanist deleted the fix/issue-210-mandatory-hed-validation branch February 18, 2026 03:45
neuromechanist added a commit that referenced this pull request Feb 18, 2026
* Add OG meta tags to demo and dashboard pages

Add Open Graph and Twitter Card meta tags so sharing
links on Slack, Discord, Twitter, etc. shows a proper
preview card with the OSA branding image from osc.earth.

* Add OSA logo, dark mode, and branded footer

- Inline SVG OSA logo in header (uses currentColor for
  dark mode compatibility)
- Dark mode CSS via prefers-color-scheme on demo page
- Footer with dynamic year, heart, and OSC link
- Consistent deep blue color palette across both pages

* Reorder footer: copyright holder first, then made with love

* Fix review issues: dark mode, meta tags, broken links

- Add dark mode to dashboard (matching frontend theme)
- Add <meta name="description"> to both pages for SEO
- Remove redundant twitter:title/description/image tags
- Fix broken GitHub links (osc-em -> OpenScience-Collective)
- Standardize org name to "OpenScience Collective"
- Move dashboard inline styles to CSS classes
- Rename .footer to .site-footer for consistency

* Enforce mandatory HED tag validation before display (#211)

* Enforce mandatory HED tag validation before display

Fixes #210

* Address review: consistent error handling, sanitize exceptions

* Fix sync status page: community-aware, all sync types, N/A display (#213)

* Fix sync status page: N/A display, all sync types, community-aware

Fixes #212

- Fix Papers Sync showing N/A: sync_metadata stores source names like
  "openalex:query", not "openalex", so use prefix matching to find the
  most recent timestamp for each source type
- Make /sync/status community-aware: accept community_id query param and
  query the correct community database (previously always used hed default)
- Add all sync types to response: new 'syncs' field includes github,
  papers, docstrings, mailman, beps, faq with last_sync and next_run
- Dashboard: pass community_id to sync status fetch, render all sync
  types dynamically including next scheduled run time

* Address review findings: validation, future times, health endpoint

- Validate community_id against registry (404 for unknown)
- Update /sync/health to accept community_id param
- Fix formatRelativeTime to handle future timestamps (next_run)
- Use _parse_iso_datetime in _get_most_recent_sync for robust comparison
- Fix misleading citing_doi comment in papers source lookup
- Add exc_info=True to warning log calls for full tracebacks
- Add tests: prefix matching regression, syncs field, unknown community 404

* Fix test: add isolated_db fixture to community_id test

* Bump version to 0.6.7.dev0

* Address PR review findings

- Fix papers last_sync timestamp comparison: use _parse_iso_datetime
  instead of string max() to avoid wrong result with mixed UTC offsets
- Fix dashboard /sync/health URL: pass community_id param so health
  badge reflects the viewed community, not always 'hed'
- Add tests for /sync/health community_id param: 404 for unknown,
  200 for known community

* Fix logging: add exc_info and upgrade scheduler warning to error

- Add exc_info=True to trigger_sync error log for full tracebacks
- Upgrade scheduler job inspection failure from warning to error
@neuromechanist neuromechanist mentioned this pull request Feb 25, 2026
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HED assistant generates unvalidated, hallucinated HED tags

1 participant