Skip to content

feat(skills): ship a ten-skill starter pack with a guide and honesty guards - #2697

Open
kovtcharov wants to merge 14 commits into
mainfrom
claudia/task-8dc71591
Open

feat(skills): ship a ten-skill starter pack with a guide and honesty guards#2697
kovtcharov wants to merge 14 commits into
mainfrom
claudia/task-8dc71591

Conversation

@kovtcharov

Copy link
Copy Markdown
Contributor

GAIA could parse and validate a SKILL.md after #2669, but there was nothing to actually look at — a new user running gaia skill list saw an empty table and a format spec. This ships ten worked skills under skills/starter/ that install, inspect, and fork today, covering web research, RAG, scratchpad SQL, memory, a connector-bridged MCP permission, and a skill that contributes its own @tool. Each one is a template to copy rather than a feature to use, which is the point: the pack argues GAIA does whatever you can describe, not ten fixed things.

Four of the ten skills proposed in #893 are deliberately not here. They describe capabilities GAIA does not have yet, and a skill that parses but cannot run is worse than a missing one:

Proposed Blocker
morning-brief gaia schedule add --skill is rejected at add time; no Gmail/Calendar/Weather connector in the catalog; TTS is not an agent tool; the Telegram adapter has no voice-note path. daily-brief ships the composition half.
voice-research Telegram handles photos and documents but not voice; TTS is CLI-only (gaia talk).
trading-alert No finance connector, and no historical price source to compute RSI from.
email-triage There is no gmail connector id — Gmail is a scope of the google OAuth connector. Email triage already ships as a full agent (gaia email); github-triage covers the same connector-bridged primitive.

Three more (source-watch, price-watch, check-in) ship rescoped to on-demand, because the scheduler cannot run skills — each says so in its own body rather than only in the guide. Scheduling is the single largest unblock. The guide carries the same table so the docs never promise more than the runtime delivers, and credits Nous Research / Hermes for the starter-pack model and agentskills.io for the format.

Based on #2669 (claudia/task-7afdc4b3), not main — retarget once that merges. Separately filed #2687 for a pre-existing IPv6 bug in WebClient found while validating the web-facing skills; left out of this PR to keep it scope-clean.

Test plan

  • python -m pytest tests/unit/test_starter_skills.py -q — 120 passed, 9 skipped. Iterates skills/starter/ so a new skill is covered without a new test.
  • Every skill parses, validates, round-trips byte-identical, and declares only network:* / mcp:connect:<id>mcp:connect resolved against the live connector catalog, so it cannot name a connector a user can't configure.
  • Every tools_required name is checked against tools the real mixin registrars produce (the Phase 1 loader does not check this, so a typo would otherwise ship silently).
  • Argument semantics are checked too, not just tool names: memory category= literals against VALID_CATEGORIES, and scratchpad SQL for the mandatory scratch_ prefix. Both guards were confirmed to fail on the bugs they catch.
  • Real CLI, cold state, HOME/GAIA_CONFIG_DIR redirected into tmp_path:
    gaia skill import skills/starter/<name>gaia skill list --jsongaia skill info <name> --json for all ten.
  • Forkability: copy source-watch, rename the directory + name, import — verified end to end. A fork that forgets to rename fails loudly.
  • rss-digest/fetch_rss run against real Atom (github.com/amd/gaia/releases.atom) and RSS 2.0 (hnrss.org/frontpage) feeds; refuses DTD-bearing feeds, loopback addresses, and unrecognized dialects instead of returning an empty digest.
  • python util/lint.py --all clean.

Closes #893

…core

Nothing in GAIA could read a SKILL.md before this. Agents gain capability only
by shipping new Python, so a user who wants a new one has no path short of
forking the agent — and the 800K+ skills already published to community
registries were unreachable. Now a folder with a SKILL.md drops into
~/.gaia/skills/ and any agent can load it at runtime, and an existing Claude
Code library under .claude/skills/ works unchanged.

Phase 1 of #1019, deliberately scoped to keep a sandbox out of it: a skill may
add instructions and reach connector-backed services, and a skill declaring a
local-capability permission (filesystem/shell/database/desktop/env) is refused
with an actionable error rather than loaded without enforcement.

- gaia.skills.format: frontmatter + body -> Skill; round-trip is identity;
  unknown top-level keys and foreign metadata.<vendor> namespaces survive a
  write. compatibility/allowed-tools parse but are ignored — never a permission
  mechanism.
- gaia.skills.manager: three v1 roots (agent-bundled -> ~/.gaia/skills ->
  read-only .claude/skills), first-wins precedence with the shadowed copy kept
  visible, progressive disclosure, FileWatcher hot-reload.
- gaia.skills.loader: tools register under <skill>/<tool>; a manifest that
  contradicts tools.py fails loudly and restores the registry byte-for-byte.
- gaia.skills.permissions: network/mcp resolve to the existing
  ConnectorRequirement — no second grant ledger.
- Agent.load_skill/unload_skill, and gaia skill list|info|create|import|export.

Closes #888
…s too

The refusal only ran in Agent.load_skill, so a caller reaching for the
lower-level register_skill_tools could hand a skill executable reach without
passing the permission gate. Move it to the function that actually registers
tools so the invariant holds at every entry point.
Five downstream issues build directly on load_skill / skill_manager /
loaded_skills; the return types were inferred, not declared.
The gate's error pointed at #888, which this branch closes; it now points at
the umbrella #1019, where wiring the scheduler to the skills runtime lives.
The unload test only checked the global registry. Assert on every surface the
model can see — the instance snapshot and the rendered tool block — and add a
reload-after-unload case so a poisoned module cache would fail. Also pin the
lazily-built manager's user root at tmp_path so no test can resolve the real
~/.gaia/skills.
/skills/ was ignored wholesale for AMD-internal marketplace installs; narrow
the rule so the shipped starter pack under skills/starter/ stays tracked.
Nine instruction-only recipe skills plus one tool-providing skill (rss-digest).
Every manifest validates against the Phase 1 parser, round-trips byte-identical,
and declares only connector-bridged permissions (network:*, mcp:connect).
Iterates skills/starter/ so a new skill is covered without a new test. Beyond
parse+validate, asserts every tools_required name resolves to a tool a mixin
really registers — the loader does not check that, so a typo would otherwise
ship a skill whose procedure references a tool the model never has.
One section per shipped skill with what it does, how to configure, and how to
install and run — plus the four proposed skills that are NOT shipped and the
specific blocker for each, so the guide never promises more than the runtime
delivers. Credits Nous Research / Hermes and agentskills.io.
…in it

The snippet called a 'query' method ChatAgent does not have. Added a test that
fails if the documented API drifts again.
Review found procedures that name real tools but call them in ways the tools
reject: check-in passed category="task" (not a valid memory category, so
remember errored and recall matched nothing), data-explore's example SQL omitted
the scratch_ table prefix every query needs, and research-report told the model
to write files without declaring write_file.

Harden rss-digest's fetch_rss too: refuse feeds carrying a DTD (stdlib
ElementTree expands entities), report unrecognized dialects instead of returning
an empty digest that reads as 'nothing published', and close the WebClient
session it opens.

New guards check argument semantics, not just tool names, and pin the guide's
per-skill tool lists to the manifests.
@github-actions github-actions Bot added documentation Documentation changes devops DevOps/infrastructure changes tests Test changes labels Jul 30, 2026
A windowed scan of the first 4 KB was escapable with a large leading comment.
Scan the whole prolog instead, and stop at the root element so a <!DOCTYPE
inside an entry's embedded HTML stays content rather than a false refusal.
@kovtcharov

Copy link
Copy Markdown
Contributor Author

Heads-up on what the green checks do and do not cover: the unit-test workflow did not run on this PR. test_unit.yml triggers on `pull_request: branches: [main], and this is stacked on #2669 (claudia/task-7afdc4b3), so the checks above are build/docs/lint only — tests/unit/test_starter_skills.py` was not executed by CI.

Run locally instead: python -m pytest tests/unit/test_starter_skills.py -q123 passed, 9 skipped (the skips are the scratchpad-SQL guard, which only applies to skills consuming query_data).

The three semantic guards were mutation-tested — each was confirmed to fail when the bug it targets is reintroduced, so they are real guards rather than tautologies.

Retargeting to main once #2669 merges will pick the workflow up; worth re-confirming green at that point before merge.

itomek
itomek previously approved these changes Jul 30, 2026
@kovtcharov kovtcharov self-assigned this Jul 30, 2026
kovtcharov-amd
kovtcharov-amd previously approved these changes Jul 30, 2026
Base automatically changed from claudia/task-7afdc4b3 to main July 31, 2026 00:46
@kovtcharov-amd
kovtcharov-amd dismissed stale reviews from itomek and themself July 31, 2026 00:46

The base branch was changed.

kovtcharov added a commit that referenced this pull request Jul 31, 2026
… gate

Skills are contributed by pull request — the hub's publish endpoint authenticates
against PUBLISH_TOKENS, a maintainer-held wrangler secret, so there is no
self-serve path and never was. That makes this workflow the primary contributor
route rather than a secondary check, and it is now built to stop a merge:

- BLOCK and unparseable fail outright, with no override. Sign-off is for the
  judgement call, never for a rejected skill.
- REVIEW fails until a maintainer applies 'skill-audit-reviewed'. Applying the
  label re-runs the check automatically, and labels need repo write access, so
  the sign-off is both auditable and maintainer-only.
- The paths filter is gone. A required check that never runs sits pending
  forever, so a path filter would have made every non-skill PR unmergeable; the
  job runs everywhere and no-ops in ~15s instead.

Sign-off unblocks the merge only: the verdict on record stays REVIEW, publishing
still refuses it, and merging is not a tier promotion. The tier stays owned by
the audit verdict plus signing, which is why the skills route deliberately
diverges from the agent route where review sets the trust level.

Contributions land in skills/community/<name>/, a sibling lane to #2697's
skills/starter/ rather than a third layout — starter/ is AMD-authored material
that exists to be copied, and a contributed skill must not be presented to
newcomers as a reference implementation nobody reviewed as one.

A contract test locks the gate's load-bearing settings, because the failure mode
is silent: reintroduce a paths filter or soften REVIEW to a warning and the
workflow still runs and still goes green, it just stops gating anything.
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Verdict: Request changes

This PR ships the Phase 1 skills runtime (gaia.skills: parser/validator, SkillManager discovery, tool loader, permission→connector bridge, the gaia skill CLI, base-Agent load_skill/unload_skill), a ten-skill starter pack, and the matching doc updates. The runtime is genuinely well built — fail-loudly errors, no-partial-load registry restoration, per-instance connector requirements, and thorough tests. One thing blocks merge.

🔒 SECURITY CONCERN: entity-expansion (billion-laughs) DoS guard is bypassable in the rss-digest starter skill. @kovtcharov-amd — the DTD guard added last commit still has a hole. It decides where the XML prolog ends by finding the first <letter, but a comment or processing instruction placed in the prolog that itself contains a <letter sequence moves that boundary inside the comment, so a <!DOCTYPE …> after it escapes the scan and reaches stdlib ElementTree, which expands entities. Because fetch_rss runs against an attacker-controlled feed URL, a small crafted feed can exhaust memory/CPU — exactly what the guard exists to prevent. The existing "prolog-padding" test doesn't catch it because its comment is all x bytes with no inner tag. Fix direction: locate the prolog boundary with comments/PIs skipped, or parse with defusedxml and drop the hand-rolled scan. Please add a regression test with a <letter-bearing comment/PI ahead of the DOCTYPE.

Real-world evidence

evidence-bundle.md is present and thorough (CLI surface, on ubuntu-latest, no inference). It exercised the real gaia skill tree end-to-end against a sandboxed HOME/GAIA_CONFIG_DIR: list cold-start, create → list → info → export → import round-trip, .zip re-import with --force, --json on stdout with logs routed to stderr, and error exit codes (3 not-found, 4 invalid). It also verified the updated gaia schedule add --skill rejection copy and the #1019 link live, and spot-checked sibling subparsers (connectors, agent) and the base-Agent import for regressions — all clean. API/MCP rows are correctly marked N/A (grep-confirmed untouched); real-inference skill use is deferred to the strix-halo lane. The evidence supports the CLI/schedule surfaces. It does not cover the rss-digest parser path above (unit-only, and the bypass class isn't in the suite), so that finding rests on static review.

🔍 Technical details

🔴 / 🔒 Prolog DTD-guard bypass — skills/starter/rss-digest/tools.py:24,108

_ROOT_START_RE = rb"<[A-Za-z]" (line 24) is used at line 108 to split prolog from body: prolog = payload[: root_start.start()]. A prolog may legally contain comments and PIs, and both can embed a <letter run (<!-- <a> -->, <?x <a>?>). The regex matches inside them, so the boundary lands mid-comment and a following <!DOCTYPE> is not in prolog → not refused → ElementTree.fromstring processes the DTD. xml.etree is documented-vulnerable to billion-laughs / quadratic-blowup expansion; the response-size cap on the fetch does not bound expansion. The test_fetch_rss_refuses_a_dtd_hidden_behind_prolog_padding case uses b"<!-- " + b"x"*8000 + b" -->" (no inner tag), so it passes while the bypass remains. Prefer defusedxml.ElementTree (kills the class outright) or skip comment/PI spans when finding the root, plus a regression test.

🟢 unload_skill doesn't reverse the connector requirements it added — src/gaia/agents/base/agent.py:950,991

load_skill appends the skill's ConnectorRequirements to the per-instance REQUIRED_CONNECTORS shadow (correctly, no ClassVar leak), but unload_skill (line 991) only removes tools + prompt body and pops loaded_skills; the connector requirements stay. An agent that loads then unloads a skill still advertises a requirement for a capability it no longer has. Symmetric removal (guarding against a requirement another loaded skill still needs) would close the asymmetry. Same note applies to the except Exception rollback in load_skill — it unregisters tools but leaves the appended requirement and the _instance_tools update in place if rebuild_system_prompt() throws.

Strengths

  • No-partial-load is real, not aspirational. loader.py snapshots _TOOL_REGISTRY + sys.modules, validates the manifest↔signature cross-check (missing/undeclared/param-name/required-flip/type), and restores byte-for-byte on any BaseException before re-raising. The identity-vs-presence diff (entry is not before[name]) correctly handles a skill tool shadowing an existing unqualified registry key.
  • Per-instance connector requirements (agent.py:952-957) avoid leaking one agent's skill grants into siblings via the ClassVar — a subtle trap avoided deliberately.
  • Fail-loudly error messages across format.py/permissions.py/manager.py name what failed, what to do, and where to look, matching GAIA's policy; mcp:connect requires a catalog-resolved scope (no ambient-authority wildcard) and mcp-github is confirmed present in the catalog.
  • Doc sync is exemplaryskill-format.mdx retracts its earlier "verified absent on main" claims, the CLI reference and docs.json nav are updated, and the starter-skills guide is honest about what Phase 1 refuses (local-capability domains) and what isn't wired (skill scheduling).
  • Path-traversal guards on both SkillManager.resource_path and .zip import (_unpack).

@kovtcharov-amd

Copy link
Copy Markdown
Collaborator

The SD check was an infra hiccup, not related to this change — re-ran it and it's green.

For the record: the models were already cached, so this wasn't a slow download. The "Initialize SD Profile" step hung for its full 10m timeout inside model verification (loading the models for a warm-up inference), which left too little of the 15m job budget for the tests and the run got cancelled. On the re-run the same step took 47s and the whole job finished in 1m37s — so the timeout budget is fine and needs no change; it was a one-off stall, most likely contention with another job on the shared Lemonade runner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

devops DevOps/infrastructure changes documentation Documentation changes tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Starter skill pack: 10 agentskills.io skills demonstrating the platform

3 participants