Skip to content

Add LTV skill for guardrails, configuration, and QC interpretation - #73

Open
cmungall wants to merge 2 commits into
mainfrom
cmungall/portable-repo-skills
Open

cmungall wants to merge 2 commits into
mainfrom
cmungall/portable-repo-skills

Conversation

@cmungall

@cmungall cmungall commented Sep 20, 2026

Copy link
Copy Markdown
Member

Add a portable LTV skill for the agent's work around deterministic ontology QC: integrating repository guardrails, interpreting findings and incomplete coverage, and configuring ontology access and validation policy. A passing check does not require an agent to repeat every lookup; an ID and canonical label can still name the wrong biological entity.

The main skill focuses on diagnosis and sound curation decisions. Separate reference guides cover hook/CI setup and configuration, using pinned examples from dismech's term wrapper, edit hook, and staged online/offline CI checks. They distinguish label and enum caches, report unchecked Not4Curation coverage, preserve constraints during repairs, and explain the different exit/strictness rules for data, schema, text, and plugin entry points. The README includes list-before-install npx skills commands and project/global scope; skill installation does not activate hooks or CI.

Validation:

  • 146 focused tests passed, five deselected (tests/test_severity_overrides.py, tests/test_offline_mode.py, tests/test_not4curation.py).
  • Eleven Typer CliRunner workflow cases passed against a local OBO fixture and documented config, covering cache misses, valid binding, unchecked offline Not4Curation data, label failures, severity thresholds, and schema-specific behavior. Confirmed that validate-schema has no --fail-on option.
  • Skill frontmatter, Markdown lint (MD013 excluded), relative/external links, and local npx skills add <worktree> --list passed. No new README lint violations relative to the previous PR head.
  • Documentation only; no hook installed or live skill environment changed. Canonical default-branch discovery becomes available after merge.

@claude

claude Bot commented Sep 20, 2026

Copy link
Copy Markdown

Claude finished @cmungall's task in 1m 43s —— View job


Review: Add portable ontology-term validation skill

  • Read PR diff (README.md, skills/linkml-term-validator/SKILL.md)
  • Verify every flag/behavior claim in SKILL.md against src/linkml_term_validator/cli.py
  • Check README install block and doc links
  • Post review

Docs-only change, no runtime code touched — no security or performance impact. The skill is well-written and unusually careful about the failure modes that matter here: the exit-code-2 ambiguity, "unchecked ≠ passed" for Not4Curation, and the --offline caveat are all accurate against the code (cli.py:125-173, oak_utils.py:544 confirms offline never builds an adapter even for a local simpleobo: file). Frontmatter name matches the directory, and the description is well within limits.

Two accuracy bugs, though — and in a skill, an inaccurate flag is a functional bug, since an agent will run it verbatim.

Must fix

1. validate-schema has no --fail-on flagSKILL.md:75-77

For validate-schema and validate-data, --fail-on controls the severity threshold

--fail-on is defined only on validate-data (cli.py:413-422) and validate (cli.py:713-722). validate_schema (cli.py:176-248) has no such option; it exits 1 iff result.error_count() > 0 (cli.py:289). An agent following this runs validate-schema --fail-on error, gets a Click usage error with exit code 2 — the exact code the next bullet warns is ambiguous with a service outage. So the skill's own mitigation gets triggered by the skill's own mistake.

Fix this →

2. --strict means two different things, and only one is documentedSKILL.md:77-78

--strict raises the threshold to include warnings when --fail-on error is used. It does not enable disabled checks.

That describes validate-data only (_effective_fail_on, cli.py:84-104; results still print as WARN, cli.py:423-432). On validate-schema, --strict promotes warnings to ERROR in the report (cli.py:192-198, "Treat all warnings as errors"), so it fails unconditionally — there's no threshold involved. validate_all's own docstring spells out the split (cli.py:741-743). As written, an agent will assume validate-schema --strict is a no-op unless paired with --fail-on error and mis-read a strict schema failure.

Worth noting the skill already draws this distinction for validate-text-file --strict (a third meaning: unresolvable CURIEs become errors, cli.py:1020-1026) — it just needs the same treatment for schema vs. data.

Fix this →

Should fix

3. Documentation link is probably a 404SKILL.md:90

Links to https://linkml.io/linkml-term-validator/, but mkdocs.yml:57 sets site_url: https://linkml.github.io/linkml-term-validator. This is the only occurrence of the linkml.io form anywhere in the repo. Fix this →

4. --lenient is narrower than describedSKILL.md:80-81

lenient is only threaded into BindingValidationPlugin(strict=not lenient) (cli.py:513-514); DynamicEnumPlugin never receives it (cli.py:495-505). "disables term-existence failures" reads as global. Scoping it to binding/label checks would keep an agent from concluding that --lenient silences dynamic-enum membership failures too.

5. Absolute-path advice conflicts with the repo's own conventionSKILL.md:20, 25

simpleobo:/absolute/path/... works, but CLAUDE.md and every test config use relative forms (simpleobo:tests/data/test_ontology.obo). Since the skill also says "reuse the project's oak_config.yaml", an agent may "correct" working relative paths. Suggest "a path to a local OBO file (relative paths resolve from the working directory)".

Nice to have

  • Coverage for the skill itself. Nothing in tests/ asserts the frontmatter parses, that name matches the directory, or that flags cited in SKILL.md exist. A ~20-line test that extracts flags from the fenced bash blocks and checks them against the Typer command's params would have caught findings 1 and 2 mechanically, and would keep the skill honest as the CLI evolves. Worth it given the skill is published for external agents.
  • migrate-cache is unmentioned (cli.py:792). It's the documented remedy after an upgrade or an ontology relabeling, which fits the skill's cache-hygiene advice. validate (auto-detect, cli.py:612) is reasonable to omit.
  • uvx linkml-term-validator floats to latest on every invocation. For repeatable term validation, uvx linkml-term-validator@<version> would pair well with the skill's own "record the adapter and cache directory with the results".
  • First-run cost of sqlite:obo:: the skill says it "can download ontology databases" — worth adding that this can be multi-GB and slow, so --cache-dir should point somewhere persistent in CI.

README

Reads well and the caveat that the skill installs instructions, not the runtime, is a good addition. I couldn't verify the skills.sh CLI flags (--list, --skill, -a, -g) from this sandbox — no network — so please confirm those against the current CLI. As you noted, --list against the default branch won't show anything until this merges.


Nothing here blocks the concept; findings 1 and 2 are the ones I'd want fixed before merge, since a published skill that names a nonexistent flag will fail for every downstream agent. I did not make any changes — this is review feedback only.
· branch cmungall/portable-repo-skills

@cmungall cmungall changed the title Add portable ontology-term validation skill Add LTV skill for guardrails, configuration, and QC interpretation Sep 20, 2026
@claude

claude Bot commented Sep 20, 2026

Copy link
Copy Markdown

Claude finished @cmungall's task in 3m 5s —— View job


Review: Add portable ontology-term validation skill (re-review after restructure)

  • Read updated PR diff (README.md, SKILL.md, references/guardrails.md, references/configuration.md)
  • Verify every CLI claim against src/linkml_term_validator/cli.py, validator.py, models.py, plugins/
  • Check frontmatter, links, README block
  • Post review

Docs-only; no runtime code touched, so no security or performance surface. The restructure into SKILL.md + two reference files is a clear improvement, and the four accuracy findings from my earlier pass are now fixed:

Prior finding Status
--fail-on wrongly attributed to validate-schema Fixed — configuration.md:68 now says it has none, and that ordinary warnings can exit 0 (matches cli.py:289-296)
--strict conflated across subcommands Fixed — the table at configuration.md:65-70 splits data / schema / text-file correctly (cli.py:423-432, validator.py:326-336, cli.py:1020-1026)
--lenient described too broadly Fixed — now scoped to term-existence failures, which is exactly right: TERM_NOT_FOUND is only emitted by binding_plugin.py:664, and dynamic_enum_plugin.py never receives strict
Absolute-path advice Fixed — configuration.md:28-36 uses a relative simpleobo: path plus a note about runner-relative resolution

I spot-checked the remaining technical claims and they hold: --config/-c (cli.py:213-220), --fail-on any as the default that fails on every result (cli.py:119-120), include_self defaulting to False so reachable_from roots are excluded (plugins/base.py:443-445), the severity_overrides: binding_label_mismatch key (models.py:77), exit 2 for "Unable to validate at this time" (cli.py:39,140), Not4Curation unchecked-terms being a non-gating note (cli.py:147-173), --offline never constructing an adapter, and the schema-side rule that label mismatches are ERROR for configured prefixes but WARNING for an unconfigured prefix until --strict promotes them (validator.py:429-440). The validate-text-file flags (--regex, --curie-group, --label-group, --strict) all match cli.py:989-1085.

Should fix

1. Documentation link is still a 404references/configuration.md:107

Unchanged from my last pass. The skill links to https://linkml.io/linkml-term-validator/, but mkdocs.yml:57 sets site_url: https://linkml.github.io/linkml-term-validator, .github/workflows/deploy-docs.yaml:57 publishes with mkdocs gh-deploy, and there's no CNAME anywhere in the repo. This is still the only linkml.io/linkml-term-validator occurrence in the tree. An agent that follows it to resolve "the installed release's plugin and binding options" lands on nothing and falls back to guessing — the failure mode the skill exists to prevent.

Fix this →

2. dismech appears in SKILL.md with no antecedentSKILL.md:47

In dismech, arbitrary qualifier fields need additional checks

SKILL.md is the always-loaded file; dismech is only introduced 90 lines into references/guardrails.md, which an agent reads only when setting up a gate. Read cold — and this skill is meant to be installed into arbitrary repos via npx skills add — "In dismech" is a dangling proper noun that an agent may try to resolve against the host project. The point it illustrates (unconstrained qualifier fields pass structurally while LTV never inspects them) stands on its own; either drop the name or phrase it as "in one inspected project (see Guardrail setup)".

Fix this →

Nice to have

  • The entry-point table omits validate (configuration.md:65-70). configuration.md:63 tells the agent to "check the installed release and exact entry point before changing policy", but the auto-detect validate command (cli.py:612) has its own --fail-on, --strict, and --lenient (cli.py:648, cli.py:713-722) and isn't in the table. A one-row addition noting it follows validate-data policy for data and validate-schema policy for schemas would close the gap.
  • migrate-cache is still unmentioned (cli.py:792). "Diagnose cache problems" says to use "the project's refresh or expansion workflow"; the built-in remedy after an upgrade or ontology relabeling is worth naming, since an agent otherwise has no sanctioned alternative to the hand-editing the section correctly forbids.
  • No test covers the skill. Nothing in tests/ asserts the frontmatter parses, that name matches the directory, or that flags cited in the skill exist as Typer params. The four fixes above were all found by hand twice; a ~20-line test extracting flags from the fenced bash blocks in skills/**/*.md and checking them against the commands' params would make that mechanical, which matters more now that the skill is published for external agents.
  • First-run cost of sqlite:obo: (configuration.md:24). "can download databases" understates a multi-GB, multi-minute first run; guardrails.md:56 gets partway there with "keep whole-ontology downloads out of every edit", but a note that --cache-dir should point somewhere persistent in CI would help.
  • External-example durability (guardrails.md:90-109). Pinning all five links to 6bd2810 is good practice. The section is still wholly dependent on an external repo staying public; the takeaways (pinned release in a wrapper, staged online-PR / offline-merge-queue coverage) would survive link rot if stated inline.

README

Reads well, and the caveat that installing the skill supplies instructions but not the runtime, hooks, or CI is the right thing to say twice. As before, I can't verify skills.sh's --list, --skill, -a, -g flags from this sandbox (no network) — please confirm against the current CLI. Worth noting -a means --adapter in LTV and --agent in the skills CLI; the surrounding prose makes that clear enough in context.


Nothing here blocks merge. Finding 1 is a stale link I'd fix before publishing, and finding 2 is a one-line reword. I did not make any changes — review feedback only.
· branch cmungall/portable-repo-skills

This branch has not been deployed

No deployments
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.

1 participant