Addressed claude interface eval #1264

.github/workflows/claude-code-review.yml

@@ -11,8 +11,9 @@ on:
 jobs:
   claude-review:
     # Avoid duplicate runs: use pull_request for same-repo, pull_request_target for forks
-    # Skip bot PRs (dependabot, renovate, etc.) - they don't need code review
+    # Skip bot PRs (dependabot, renovate, etc.) and draft PRs
     if: |
+      !github.event.pull_request.draft &&
       github.event.pull_request.user.type != 'Bot' &&
       !(github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork) &&
       !(github.event_name == 'pull_request_target' && !github.event.pull_request.head.repo.fork)
@@ -28,7 +29,10 @@ jobs:
       - name: Checkout repository
         uses: actions/checkout@v6
         with:
-          ref: ${{ github.event.pull_request.head.sha }}
+          # Use base.sha, not head.sha: checking out fork code under
+          # pull_request_target would give untrusted code access to secrets.
+          # Claude reads changes via gh pr diff (API), not the local checkout.
+          ref: ${{ github.event.pull_request.base.sha }}
           fetch-depth: 1
 
       - name: Run Claude Code Review

hed/__init__.py

@@ -1,9 +1,8 @@
 from hed.models.hed_string import HedString
 from hed.models.hed_tag import HedTag
+from hed.models.hed_group import HedGroup
 from hed.errors.error_reporter import get_printable_issue_string
 from hed.errors.exceptions import HedFileError, HedExceptions, HedQueryError
-
-from hed.models.base_input import BaseInput
 from hed.models.spreadsheet_input import SpreadsheetInput
 from hed.models.tabular_input import TabularInput
 from hed.models.sidecar import Sidecar

hed/errors/__init__.py

@@ -4,6 +4,8 @@
     ErrorHandler,
     separate_issues,
     get_printable_issue_string,
+    get_printable_issue_string_html,
+    check_for_any_errors,
     sort_issues,
     iter_errors,
 )
@@ -12,9 +14,11 @@
     TemporalErrors,
     SchemaErrors,
     SchemaWarnings,
+    SchemaAttributeErrors,
     SidecarErrors,
     ValidationErrors,
     ColumnErrors,
+    TagQualityErrors,
 )
 from .error_types import ErrorContext, ErrorSeverity
-from .exceptions import HedExceptions, HedFileError
+from .exceptions import HedExceptions, HedFileError, HedQueryError

hed/errors/error_types.py

@@ -1,8 +1,10 @@
 """Error codes used in different error messages."""
 
+from enum import IntEnum
 
-class ErrorSeverity:
-    """Severity codes for errors"""
+
+class ErrorSeverity(IntEnum):
+    """Severity codes for errors."""
 
     ERROR = 1
     WARNING = 10
@@ -95,7 +97,7 @@ class ValidationErrors:
 
     INVALID_VALUE_CLASS_CHARACTER = "INVALID_VALUE_CLASS_CHARACTER"
     INVALID_VALUE_CLASS_VALUE = "INVALID_VALUE_CLASS_VALUE"
-    INVALID_TAG_CHARACTER = "invalidTagCharacter"
+    INVALID_TAG_CHARACTER = "INVALID_TAG_CHARACTER"
 
     HED_PLACEHOLDER_OUT_OF_CONTEXT = "HED_PLACEHOLDER_OUT_OF_CONTEXT"
     CURLY_BRACE_UNSUPPORTED_HERE = "CURLY_BRACE_UNSUPPORTED_HERE"
@@ -105,11 +107,11 @@ class ValidationErrors:
 
 class SidecarErrors:
     # These are for json sidecar validation errors(sidecars can also produce most normal validation errors)
-    BLANK_HED_STRING = "blankValueString"
-    WRONG_HED_DATA_TYPE = "wrongHedDataType"
-    INVALID_POUND_SIGNS_VALUE = "invalidNumberPoundSigns"
-    INVALID_POUND_SIGNS_CATEGORY = "tooManyPoundSigns"
-    UNKNOWN_COLUMN_TYPE = "sidecarUnknownColumn"
+    BLANK_HED_STRING = "BLANK_HED_STRING"
+    WRONG_HED_DATA_TYPE = "WRONG_HED_DATA_TYPE"
+    INVALID_POUND_SIGNS_VALUE = "INVALID_POUND_SIGNS_VALUE"
+    INVALID_POUND_SIGNS_CATEGORY = "INVALID_POUND_SIGNS_CATEGORY"
+    UNKNOWN_COLUMN_TYPE = "UNKNOWN_COLUMN_TYPE"
     SIDECAR_HED_USED = "SIDECAR_HED_USED"
     SIDECAR_NA_USED = "SIDECAR_NA_USED"
     SIDECAR_BRACES_INVALID = "SIDECAR_BRACES_INVALID"
@@ -128,7 +130,7 @@ class SchemaWarnings:
 
     # The actual reported error for the above two
     SCHEMA_CHARACTER_INVALID = "SCHEMA_CHARACTER_INVALID"
-    SCHEMA_INVALID_CAPITALIZATION = "invalidCaps"
+    SCHEMA_INVALID_CAPITALIZATION = "SCHEMA_INVALID_CAPITALIZATION"
     SCHEMA_NON_PLACEHOLDER_HAS_CLASS = "SCHEMA_NON_PLACEHOLDER_HAS_CLASS"
     SCHEMA_PROLOGUE_CHARACTER_INVALID = "SCHEMA_PROLOGUE_CHARACTER_INVALID"
 
@@ -165,9 +167,9 @@ class SchemaAttributeErrors:
 
 class DefinitionErrors:
     # These are all DEFINITION_INVALID errors
-    WRONG_NUMBER_PLACEHOLDER_TAGS = "wrongNumberPlaceholderTags"
-    DUPLICATE_DEFINITION = "duplicateDefinition"
-    INVALID_DEFINITION_EXTENSION = "invalidDefExtension"
+    WRONG_NUMBER_PLACEHOLDER_TAGS = "WRONG_NUMBER_PLACEHOLDER_TAGS"
+    DUPLICATE_DEFINITION = "DUPLICATE_DEFINITION"
+    INVALID_DEFINITION_EXTENSION = "INVALID_DEFINITION_EXTENSION"
     DEF_TAG_IN_DEFINITION = "DEF_TAG_IN_DEFINITION"
     NO_DEFINITION_CONTENTS = "NO_DEFINITION_CONTENTS"
     PLACEHOLDER_NO_TAKES_VALUE = "PLACEHOLDER_NO_TAKES_VALUE"

hed/errors/exceptions.py

@@ -7,11 +7,11 @@ class HedExceptions:
     GENERIC_ERROR = "GENERIC_ERROR"
     # A list of all exceptions that can be generated by the hedtools.
     URL_ERROR = "URL_ERROR"
-    FILE_NOT_FOUND = "fileNotFound"
-    BAD_PARAMETERS = "badParameters"
-    CANNOT_PARSE_XML = "cannotParseXML"
-    CANNOT_PARSE_JSON = "cannotParseJson"
-    INVALID_EXTENSION = "invalidExtension"
+    FILE_NOT_FOUND = "FILE_NOT_FOUND"
+    BAD_PARAMETERS = "BAD_PARAMETERS"
+    CANNOT_PARSE_XML = "CANNOT_PARSE_XML"
+    CANNOT_PARSE_JSON = "CANNOT_PARSE_JSON"
+    INVALID_EXTENSION = "INVALID_EXTENSION"
     INVALID_HED_FORMAT = "INVALID_HED_FORMAT"
 
     INVALID_DATAFRAME = "INVALID_DATAFRAME"
@@ -24,7 +24,7 @@ class HedExceptions:
     SCHEMA_SECTION_MISSING = "SCHEMA_SECTION_MISSING"
     SCHEMA_INVALID = "SCHEMA_INVALID"
 
-    WIKI_SEPARATOR_INVALID = "invalidSectionSeparator"
+    WIKI_SEPARATOR_INVALID = "WIKI_SEPARATOR_INVALID"
 
     # This issue will contain a list of lines with issues.
     WIKI_DELIMITERS_INVALID = "WIKI_DELIMITERS_INVALID"

hed/models/__init__.py

@@ -48,10 +48,9 @@
 """
 
 from .base_input import BaseInput
-from .column_mapper import ColumnMapper
 from .column_metadata import ColumnMetadata, ColumnType
 from .definition_dict import DefinitionDict
-from .definition_entry import DefinitionEntry
+from .model_constants import DefTagNames, TopTagReturnType
 from .query_handler import QueryHandler
 from .query_service import get_query_handlers, search_hed_objs
 from .hed_group import HedGroup

hed/models/column_mapper.py

@@ -16,10 +16,28 @@
 
 
 class ColumnMapper:
-    """Mapping of a base input file columns into HED tags.
+    """Translates tabular file columns into HED tag streams for validation and analysis.
+
+    ``ColumnMapper`` is the low-level engine behind :class:`~hed.models.TabularInput` and
+    :class:`~hed.models.SpreadsheetInput`.  It resolves column definitions from a
+    :class:`~hed.models.Sidecar` and/or explicit parameters into a per-column transform
+    pipeline that produces HED strings row-by-row.
+
+    **Use this class directly when you need to:**
+
+    - Build a custom tabular reader that doesn't subclass :class:`~hed.models.BaseInput`.
+    - Inspect or override column mappings before validating (e.g. dynamic column
+      selection at runtime).
+    - Reuse a single mapper across many DataFrames for performance.
+
+    **For the common case** (reading a BIDS events file), prefer
+    :class:`~hed.models.TabularInput` which wraps ``ColumnMapper`` automatically.
 
     Notes:
-        - All column numbers are 0 based.
+        - All column numbers are 0-based.
+        - The ``column_prefix_dictionary`` parameter is treated as a shorthand for
+          creating value columns: ``{"col": "Description"}`` becomes
+          ``{"col": "Description/#"}`` internally.
     """
 
     def __init__(

hed/models/definition_entry.py

@@ -7,7 +7,29 @@
 
 
 class DefinitionEntry:
-    """A single definition."""
+    """Stores the resolved contents of a single HED Definition.
+
+    A ``DefinitionEntry`` is created when a ``Definition/`` tag group is parsed
+    and stored in a :class:`~hed.models.DefinitionDict`.  It captures:
+
+    - **name** — the lower-cased label portion (without ``Definition/``).
+    - **contents** — the inner :class:`~hed.models.HedGroup` of the definition
+      (``None`` if the definition body is empty).
+    - **takes_value** — whether exactly one tag inside contains a ``#`` placeholder
+      (i.e. the definition expects a run-time value via ``Def/name/value``).
+    - **source_context** — the error-context stack captured at parse time, used
+      to produce precise error messages when the definition is later expanded.
+
+    **Use this class directly when you need to:**
+
+    - Iterate over a :class:`~hed.models.DefinitionDict` and inspect individual
+      definition bodies or their placeholder status.
+    - Build tooling that expands, serialises, or analyses HED definitions
+      programmatically.
+
+    **Most users never need this class** — :meth:`~hed.models.DefinitionDict.get_def_entry`
+    and :meth:`~hed.models.DefinitionDict.expand_def_tag` handle the common workflows.
+    """
 
     def __init__(self, name, contents, takes_value, source_context):
         """Initialize info for a single definition.

hed/models/hed_string.py

@@ -5,7 +5,7 @@
 import copy
 from hed.models.hed_group import HedGroup
 from hed.models.hed_tag import HedTag
-from hed.models.model_constants import DefTagNames
+from hed.models.model_constants import DefTagNames, TopTagReturnType
 
 
 class HedString(HedGroup):
@@ -128,7 +128,9 @@ def remove_definitions(self):
 
         This does not validate definitions and will blindly removing invalid ones as well.
         """
-        definition_groups = self.find_top_level_tags({DefTagNames.DEFINITION_KEY}, include_groups=1)
+        definition_groups = self.find_top_level_tags(
+            {DefTagNames.DEFINITION_KEY}, include_groups=TopTagReturnType.GROUPS
+        )
         if definition_groups:
             self.remove(definition_groups)
 
@@ -362,10 +364,11 @@ def find_top_level_tags(self, anchor_tags, include_groups=2) -> list:
 
         Parameters:
             anchor_tags (container):  A list/set/etc. of short_base_tags to find groups by.
-            include_groups (0, 1 or 2):  Parameter indicating what return values to include.
-                If 0: return only tags.
-                If 1: return only groups.
-                If 2 or any other value: return both.
+            include_groups (TopTagReturnType or int):  Controls what is returned.
+                Use :class:`~hed.models.TopTagReturnType` constants for clarity.
+                ``TAGS`` (0): return only anchor tags.
+                ``GROUPS`` (1): return only groups.
+                ``BOTH`` (2, default): return ``(tag, group)`` pairs.
 
         Returns:
             list: The returned result depends on include_groups.

hed/models/model_constants.py

@@ -1,5 +1,21 @@
 """Defined constants for definitions, def labels, and expanded labels."""
 
+from enum import IntEnum
+
+
+class TopTagReturnType(IntEnum):
+    """Return-type selector for :meth:`~hed.models.HedString.find_top_level_tags`.
+
+    Attributes:
+        TAGS: Return only the anchor :class:`~hed.models.HedTag` objects.
+        GROUPS: Return only the top-level :class:`~hed.models.HedGroup` objects.
+        BOTH: Return ``(tag, group)`` pairs (default).
+    """
+
+    TAGS = 0
+    GROUPS = 1
+    BOTH = 2
+
 
 class DefTagNames:
     """Source names for definitions, def labels, and expanded labels."""

hed/schema/__init__.py

@@ -41,9 +41,7 @@
 """
 
 from .hed_schema import HedSchema
-from .hed_schema_entry import HedSchemaEntry, UnitClassEntry, UnitEntry, HedTagEntry
 from .hed_schema_group import HedSchemaGroup
-from .hed_schema_section import HedSchemaSection
 from .hed_schema_io import load_schema, load_schema_version, from_string, get_hed_xml_version, from_dataframes
 from .hed_schema_constants import HedKey, HedSectionKey
 from .hed_cache import cache_xml_versions, get_hed_versions, set_cache_directory, get_cache_directory

hed/schema/hed_schema_entry.py

@@ -10,10 +10,30 @@
 
 
 class HedSchemaEntry:
-    """A single node in a HedSchema.
+    """A single node in the HED schema vocabulary.
 
-    The structure contains all the node information including attributes and properties.
+    Every term, unit, unit class, value class, attribute, and property that
+    appears in a loaded :class:`~hed.schema.HedSchema` is represented as a
+    ``HedSchemaEntry`` (or one of its subclasses).  The entry stores the node's
+    name, all declared attributes (e.g. ``takesValue``, ``allowedCharacter``),
+    its description, and a back-reference to its containing
+    :class:`~hed.schema.HedSchemaSection`.
 
+    Concrete subclasses add section-specific state:
+
+    - :class:`HedTagEntry` — vocabulary tag nodes.
+    - :class:`UnitClassEntry` — unit class nodes (e.g. *time*, *mass*).
+    - :class:`UnitEntry` — individual unit nodes (e.g. *second*, *gram*).
+
+    **Use this class (or its subclasses) directly when you need to:**
+
+    - Introspect schema vocabulary (e.g. list all tags with ``takesValue``).
+    - Build schema validators, schema browsers, or schema-diff tools.
+    - Implement custom HED annotation tooling that looks up tag metadata.
+
+    **Most users never need this class** — :meth:`~hed.schema.HedSchema.get_tag_entry`
+    and :meth:`~hed.schema.HedSchema.get_all_schema_tags` are sufficient for the
+    common lookup patterns.
     """
 
     def __init__(self, name, section):
@@ -142,7 +162,25 @@ def _compare_attributes_no_order(left, right):
 
 
 class UnitClassEntry(HedSchemaEntry):
-    """A single unit class entry in the HedSchema."""
+    """A unit class node in the HED schema (e.g. *time*, *mass*, *frequency*).
+
+    Extends :class:`HedSchemaEntry` with the set of :class:`UnitEntry` objects
+    that belong to the class and a pre-computed ``derivative_units`` dict that
+    maps every accepted surface form (including SI prefixes and plurals) to its
+    canonical :class:`UnitEntry`.
+
+    Typical access pattern::
+
+        unit_class = schema.get_tag_entry("time", HedSectionKey.UnitClasses)
+        for name, unit in unit_class.units.items():
+            print(name, unit.attributes)
+
+    Attributes:
+        units (dict[str, UnitEntry]): Map from unit name to entry after
+            :meth:`finalize_entry` is called.
+        derivative_units (dict[str, UnitEntry]): Map from every accepted
+            surface form (plural, SI-prefixed, etc.) to the base unit entry.
+    """
 
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
@@ -216,7 +254,19 @@ def get_derivative_unit_entry(self, units):
 
 
 class UnitEntry(HedSchemaEntry):
-    """A single unit entry with modifiers in the HedSchema."""
+    """A single unit node in the HED schema (e.g. *second*, *gram*, *hertz*).
+
+    Extends :class:`HedSchemaEntry` with the list of SI unit modifiers that
+    apply to this unit, a pre-computed ``derivative_units`` mapping (surface
+    form → conversion factor), and a back-reference to the parent
+    :class:`UnitClassEntry`.
+
+    Attributes:
+        unit_modifiers (list[HedSchemaEntry]): SI modifier entries (e.g. *milli*, *kilo*).
+        derivative_units (dict[str, float]): Map from every accepted surface
+            form to its numeric conversion factor relative to the SI base unit.
+        unit_class_entry (UnitClassEntry): The parent unit class.
+    """
 
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
@@ -270,7 +320,26 @@ def get_conversion_factor(self, unit_name):
 
 
 class HedTagEntry(HedSchemaEntry):
-    """A single tag entry in the HedSchema."""
+    """A vocabulary tag node in the HED schema.
+
+    Extends :class:`HedSchemaEntry` with full/short tag name forms, value-class
+    and unit-class associations, and helper methods for tag-path traversal.
+
+    Typical access pattern::
+
+        entry = schema.get_tag_entry("Sensory-event")
+        print(entry.long_tag_name)   # "Event/Sensory-event"
+        print(entry.takes_value_child)  # child "#" entry if tag takes a value
+
+    Attributes:
+        unit_classes (dict[str, UnitClassEntry]): Unit classes accepted by this
+            tag\'s value (non-empty only if ``takesValue`` is set).
+        value_classes (dict[str, HedSchemaEntry]): Value classes that constrain
+            the value format.
+        long_tag_name (str): The full slash-separated path from the schema root,
+            with any trailing ``/#`` stripped.
+        short_tag_name (str): The final component of the tag path (short form).
+    """
 
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)