v0.3 "Foundations"¶
Specifications for the v0.3 roadmap items, in the same order as the table in the
roadmap. Milestone: v0.3.
Add the GitHub issue number to a heading once the item is filed.
1. Cache SKILL.md content in providers¶
Labels: theme:performance, type:feature, package:fs, package:http
Problem¶
LocalFileSystemSkillProvider and HTTPStaticFileSkillProvider re-fetch and re-parse
SKILL.md on every call to get_metadata() or get_body(). A single skill is read up
to five times in one agent session:
| # | Code path | Trigger |
|---|---|---|
| 1 | validate_skill() → skill.get_body() |
Registration |
| 2 | validate_skill() → skill.get_metadata() |
Registration |
| 3 | registry.get_skills_catalog() → skill.get_metadata() |
System prompt / MCP resource read |
| 4 | tool call → skill.get_metadata() |
Agent turn |
| 5 | tool call → skill.get_body() |
Agent turn |
For the HTTP provider that is five network round-trips for one file. For the filesystem provider the I/O is cheaper but the redundant YAML parsing remains.
Proposal¶
Add a content cache keyed by skill_id to the internal _get_skill_md() / _read_skill_md()
helper in both providers.
self._skill_md_cache: dict[str, str] = {}
async def _get_skill_md(self, skill_id: str) -> str:
if skill_id not in self._skill_md_cache:
self._skill_md_cache[skill_id] = await self._fetch(...)
return self._skill_md_cache[skill_id]
Cache is per provider instance. No public API change.
For HTTP, prefer conditional requests (If-None-Match / If-Modified-Since using the
stored ETag / Last-Modified) over a blind TTL — a 304 costs one cheap round-trip and
keeps long-lived servers correct when a skill is republished. Expose an explicit
invalidate(skill_id=None) for callers that need it.
Acceptance criteria¶
- [x] HTTP provider issues exactly one request per skill across a full register → catalog → tool-call flow (assert on mocked
httpxcall count) - [x] Filesystem provider reads each
SKILL.mdonce (spy onPath.read_text) - [x] Cache is per-instance — two providers do not share state
- [x]
invalidate()clears one skill or all - [x] HTTP revalidates with
ETag/Last-Modifiedand handles304 - [x] Existing tests pass unchanged
Implementation notes¶
Status: implemented, awaiting release.
Two acceptance criteria conflict: "exactly one request per skill" and "revalidates with
ETag/Last-Modified" cannot both hold under one default, because revalidation costs a
round-trip per access even when it returns 304. Resolved by making revalidation opt-in:
HTTPStaticFileSkillProvider(..., revalidate=True). The default serves cached content until
invalidate() is called, which is right for the common case (a static host, a process that
outlives a few agent turns). Long-lived servers fronting a mutable host should opt in.
Only SKILL.md is cached. Scripts, assets and references are left uncached: they are larger,
typically read once, and caching them would make the provider's memory footprint depend on
untrusted content size.
Related follow-ups (file separately if wanted)¶
validate_skill()callsget_body()andget_metadata()independently, parsing twice. Fetch and split once.SkillRegistry.get_skills_catalog()re-derives metadata for every skill on each call; the registry has nounregister(), so the catalog string can be memoised.- Both providers duplicate
_get_skill_md()→split_frontmatter(); a shared helper returningtuple[dict, str]avoids double parsing.
2. Build the skills catalog concurrently¶
Labels: theme:performance, type:feature, package:core
Problem¶
SkillRegistry.get_skills_catalog() awaits get_metadata() for each skill in sequence. With
an HTTP-backed registry of N skills that is N serial round-trips on the critical path of the
first agent turn.
Proposal¶
Fan out with asyncio.gather and a bounded semaphore (default concurrency ~8, configurable).
Preserve deterministic ordering by sorting results after the gather, not relying on completion
order.
Acceptance criteria¶
- [x] Catalog output is byte-identical to the serial implementation for the same registry
- [x] Metadata fetches run concurrently (assert with an instrumented provider)
- [x] Concurrency is bounded and configurable
- [x] A failure in one skill's metadata surfaces with the offending
skill_idin the message
Implementation notes¶
Status: implemented, awaiting release.
Concurrency is bounded by an asyncio.Semaphore and configured per registry:
SkillRegistry(catalog_concurrency=8). Values below 1 raise ValueError.
Ordering is preserved by zipping the asyncio.gather results back against list_skills()
rather than sorting afterwards — gather already returns results in argument order, so no
sort is needed and the catalog stays byte-identical to the serial version.
Skill ID context is attached with exc.add_note() (PEP 678) rather than by wrapping in a new
exception. Wrapping would have changed the exception type callers see, breaking anyone
catching SkillNotFoundError from a catalog build.
3. Stop blocking the event loop in the filesystem provider¶
Labels: theme:performance, type:bug, package:fs
Problem¶
LocalFileSystemSkillProvider exposes an async interface but reads synchronously via
Path.read_text() / read_bytes(). Every skill read blocks the event loop, which matters for
servers handling concurrent agent sessions (including the MCP server in streamable-HTTP mode).
Proposal¶
Move blocking I/O to a worker thread (asyncio.to_thread, or anyio.to_thread.run_sync to
stay loop-agnostic). Keep the size check before the read where possible so oversized files are
rejected without loading them.
Acceptance criteria¶
- [x] No blocking filesystem calls on the event loop
- [x] Size limit still enforced
- [x] Concurrent reads from one provider work correctly
Implementation notes¶
Status: implemented, awaiting release.
Used asyncio.to_thread rather than anyio.to_thread.run_sync. Loop-agnosticism was not
worth a new runtime dependency on a provider package whose only current dependency is
agentskills-core; the rest of the SDK is asyncio-based already. Revisit if anyone asks for
Trio support.
The whole helper moved to the worker thread, not just the read_text() call. The
path-traversal guard (Path.resolve(), is_dir(), is_file(), stat()) issues syscalls
too, and on a cold cache or a network mount those dominate. Moving only the read would have
left most of the blocking behind.
4. Add resource discovery to the provider contract¶
Labels: theme:correctness, type:feature, package:core, package:fs, package:http
Status: implemented. The
breaking-changelabel was dropped — the chosen design adds no abstract method, so existing third-party providers keep working unchanged.
Problem¶
There is no way for an agent to learn which references, scripts, or assets a skill contains.
get_reference(skill_id, name) requires a name the agent can only obtain if SKILL.md happens
to mention it in prose. Progressive disclosure is effectively half-blind: the agent can fetch
a resource but cannot enumerate one.
Proposal¶
Add to SkillProvider:
async def list_resources(self, skill_id: str) -> dict[str, list[str]]:
"""Return available resource names grouped by kind: references, scripts, assets."""
- Filesystem: list the
references/,scripts/,assets/subdirectories, applying the same identifier-safety rules used on read. - HTTP: static servers cannot be enumerated. Suggest an optional per-skill
index.jsonmanifest — which also gives the supply-chain work a natural home for integrity hashes. - Surface as a
list_skill_resourcestool in each integration and as an MCP resource.
Open question¶
Adding a required abstract method is a breaking change for any third-party SkillProvider.
Options:
- Required abstract method — clean contract, breaks implementors.
- Default implementation returning
{}plus asupports_resource_listingcapability flag — backward-compatible, weaker contract.
Recommend (2) pre-1.0, revisiting at the API freeze.
Acceptance criteria¶
- [x] Decision recorded as an ADR before implementation
- [x]
list_resources()on the provider contract - [x] Filesystem implementation, with traversal guards
- [x] HTTP behaviour decided and implemented
- [x] Exposed through LangChain, Agent Framework, and MCP integrations
- [x] Documented in each package README
Implementation notes¶
Deviation from option 2: the default raises, it does not return {}.
A default returning {} makes "I cannot enumerate this skill" indistinguishable from "this
skill has no resources". That is the same failure class as the binary-corruption bug in item 5
— a silently wrong answer is worse than a loud one, because nothing downstream can detect it.
The default now raises ResourceListingNotSupportedError, which subclasses both
AgentSkillsError and NotImplementedError so callers can catch it from either hierarchy.
The capability flag from option 2 is kept, so callers can branch before paying for a call.
Recorded in ADR 0002 along with the alternatives rejected (mixin ABCs cannot express a
per-instance capability, which the HTTP provider needs).
The flag is a plain class attribute, not a ClassVar. The HTTP provider's capability
depends on its constructor argument, not on its type.
Filesystem: skip, do not raise. Dotfiles, subdirectories, and symlinks resolving outside
the root are silently omitted. Raising would let one stray .DS_Store or editor swapfile make
a whole skill unlistable. The stricter invariant that matters is the inverse one: listing must
never return a name that a subsequent get_reference() would reject.
HTTP: manifest is opt-in, and a missing manifest is also "not supported". index.json is
read only when the provider is constructed with resource_manifest=True. If that is set but a
given skill has no manifest, the 404 is translated to ResourceListingNotSupportedError rather
than an empty listing — same reasoning as above. Manifest entries are host-supplied data that
get interpolated into URLs, so names failing the existing identifier check are dropped rather
than trusted.
Integrations return, not raise, on unsupported. The list_skill_resources tool catches
ResourceListingNotSupportedError and returns {"supported": false, "note": "..."}. For an
LLM caller, "this cannot be listed" is actionable information (fall back to the skill body),
whereas a tool error invites a retry loop. The MCP server exposes the same data as a resource
template, skills://{skill_id}/resources.
Observation for v0.4. Three of the six tools in every integration are now near-identical
skill_id/name wrappers over encode_resource_content(). The tool layer is ready to be
generated from a single table rather than hand-written per framework.
5. Stop corrupting binary skill resources¶
Labels: theme:correctness, type:bug, package:core, package:langchain, package:agentframework, package:mcp-server
Problem¶
All three integrations decode provider bytes the same way:
return (await skill.get_asset(name)).decode("utf-8", errors="replace")
For any non-text resource — a PNG, a PDF, a zip — errors="replace" turns the payload into a
run of U+FFFD replacement characters. The data is destroyed, no exception is raised, and the
agent receives garbage it cannot distinguish from a legitimately odd document. The package
READMEs currently document this as a "Note", which presents silent data loss as intended
behaviour.
The skill spec allows assets/ to hold arbitrary files, so this is reachable through the
documented happy path rather than an edge case.
Proposal¶
A shared encode_resource_content() in agentskills-core, used by all three integrations so
the behaviour cannot drift apart again:
- Sniff for binary content (null byte within the first 8 KiB).
- If it looks like text, attempt a strict UTF-8 decode and return the string on success.
- Otherwise return a JSON envelope carrying the media type (via
mimetypes) and base64 content:
{
"name": "architecture.png",
"media_type": "image/png",
"size_bytes": 20481,
"encoding": "base64",
"content": "iVBORw0KGgo..."
}
Base64 costs roughly 1.37x the raw bytes in characters, and a 1 MiB image would swamp a context
window. Above a max_inline_binary_bytes ceiling (default 64 KiB, caller-overridable) return
the same envelope with "encoding": "none" and a note explaining the omission, so the agent
learns the resource exists and why it was not inlined.
The strict decode in step 2 matters: it is what converts a silent corruption into a detected
one. Falling back to the envelope on UnicodeDecodeError means no input path can produce
replacement characters.
Open question¶
MCP supports typed binary content blocks and resource links natively, so the JSON envelope is a lowest-common-denominator choice there. Options: ship the envelope everywhere first for consistency and specialise the MCP server afterwards, or diverge immediately. The envelope is the smaller change and keeps one code path; the MCP specialisation is worth doing but is a separate piece of work.
Acceptance criteria¶
- [x] No integration calls
.decode("utf-8", errors="replace")on resource bytes - [x] A PNG asset round-trips:
base64.b64decode(envelope["content"]) == original_bytes - [x]
media_typeinferred from the resource name - [x] UTF-8 text resources unchanged, including non-ASCII
- [x] Text that is valid UTF-8 but contains a null byte still returns intact
- [x] Oversized binaries return
"encoding": "none"with an explanatory note, not truncated content - [x]
max_inline_binary_bytesoverridable onget_tools()andcreate_mcp_server() - [x] READMEs describe the envelope; the existing "replacement characters" note is removed
Implementation notes¶
Status: implemented, awaiting release.
The null-byte sniff in step 1 of the proposal was dropped. It contradicts the fifth acceptance
criterion — valid UTF-8 containing a null byte would have been classified binary and wrapped in
an envelope. The implementation is decode-first: attempt a strict UTF-8 decode, fall back to the
envelope on UnicodeDecodeError. That is also cheaper than it looks, because a strict decode
fails at the first invalid byte rather than scanning the whole payload. One code path, and no
input can produce a replacement character.
The ceiling comparison is len(data) > max_inline_binary_bytes, so a resource exactly at the
limit is still inlined. max_inline_binary_bytes=0 never inlines; negative values raise
ValueError.
Two existing tests asserted the corruption was intended —
test_binary_content_decoded_with_replacement in the LangChain and Agent Framework suites
asserted "\ufffd" in result. Both were replaced with round-trip assertions. Worth noting as a
pattern: the bug was covered by tests, documented in three READMEs, and still a bug.
Follow-up¶
Once the envelope lands, have create_mcp_server() return binary resources as native MCP
binary content blocks — or a resource link when oversized — rather than a JSON string the client
has to parse. Text behaviour unchanged, and encode_resource_content() stays as-is for the
non-MCP integrations.
6. Optional version field in skill frontmatter¶
Labels: theme:correctness, type:feature, package:core
Status: implemented. Ships as a breaking change for skills that already carry a non-semver
version— see the implementation notes.
Problem¶
Skills have no version. Consumers cannot pin, compare, or detect drift, and any system that publishes skills centrally needs versioned publishing and subscription pinning.
Proposal¶
Optional version frontmatter field, validated as semver when present, surfaced through
get_metadata() and included in the catalog. Fully backward-compatible — absence is valid.
Raise the field with the upstream spec rather than shipping a silent proprietary extension.
Acceptance criteria¶
- [x]
versionvalidated as semver when present, ignored when absent - [x] Returned by
get_metadata() - [x] Invalid semver fails registration with a clear message
- [x] Documented as an optional, non-spec field pending upstream adoption
Pre-implementation notes¶
The criteria contradict each other. "Fully backward-compatible" and "invalid semver fails
registration" cannot both hold: any skill already carrying a non-semver version field
registers today and will stop registering. That is the right trade-off, but it is a breaking
change for those skills and should be stated as one rather than described as compatible.
YAML coerces the field before we ever see it. version: 1.0 parses as a float,
version: 1 as an int, and version: 2024-01-15 as a datetime.date. A validator that
assumes str will raise an opaque TypeError on the three most likely authoring mistakes.
Non-str values need an explicit error telling the author to quote the value.
No new dependency. agentskills-core depends only on pyyaml; use the official semver
regex from semver.org rather than pulling in a package for one match.
Implementation notes¶
This is a breaking change, and the issue text was wrong to call it compatible. Skills with
no version are unaffected — that part holds. But version was previously an unknown key:
validate_skill() logged a warning and registered the skill anyway. Anything carrying
version: 1.0, version: v2, or a date now fails registration outright. The alternative —
warn and continue — was rejected. A version that may or may not be parseable cannot be used
for pinning or comparison, which is the entire reason for the field; a lenient validator would
ship the syntax without the guarantee.
get_metadata() needed no change. It already returns the parsed frontmatter dict verbatim,
so the field surfaced the moment it became legal. The criterion was satisfied by validation
alone. Worth noting for future frontmatter fields: the only real work is validation and
catalog rendering.
Type errors are more likely than syntax errors, so they get the better message. A non-str
value reports the actual parsed type and its repr, names YAML coercion as the cause, and shows
the corrected line — version: "1.0.0". Reporting "expected str, got float" would have been
technically accurate and practically useless, because the author wrote something that looks
like a version and has no reason to suspect the parser. This is the same failure mode as the
existing _OPTIONAL_FIELDS type loop, which is why version is validated separately rather
than added to that table.
Catalog rendering is conditional. <version> and - **Version**: are emitted only when the
field is set. The catalog goes into the system prompt on every turn, so unversioned skills must
not pay for a feature they do not use.
validate_version() is exported. A publishing layer needs to validate a version at publish
time, before a Skill object exists. Keeping the rule in one place is cheaper than having every
caller reimplement the regex and drift from it.
Observations¶
The semver regex is inlined, and that is a deliberate maintenance liability. It is stable —
semver 2.0.0 has not changed since 2013 — but if the SDK ever needs comparison (range
matching, "is this newer than that") rather than validation, a regex will not do it and the
semver package should be adopted then. Validation alone does not justify the dependency.
_KNOWN_KEYS and _OPTIONAL_FIELDS are now out of step. _KNOWN_KEYS is derived from
_OPTIONAL_FIELDS plus a literal set that now carries version. A third specially-validated
field will make this genuinely confusing. When that happens the field table should become
{name: validator} rather than {name: type}, which would fold the required fields, the
optional type checks, and version into one declaration.
7. Distinguish "not found" from "unavailable" in the HTTP provider¶
Labels: theme:resilience, type:bug, theme:security, package:core, package:http
Status: implemented. Scope grew to include
agentskills-core(the newSkillUnavailableError) and a credential leak confirmed during the work.
Problem¶
HTTPStaticFileSkillProvider maps every non-2xx response — and every transport failure — to
SkillNotFoundError. A 503, a DNS failure, and a genuine 404 are indistinguishable to the
caller, so nobody can implement sensible retry or alerting. A transient blip currently looks
like a missing skill.
Proposal¶
- New
SkillUnavailableError(AgentSkillsError)in core for transient/transport failures. 404/410→SkillNotFoundError.5xx,429, timeouts, connection errors →SkillUnavailableError.401/403get a distinct message that does not echo credentials.- Bounded retry with exponential backoff and jitter for retryable classes only; honour
Retry-After. Retry count and timeout configurable on the constructor.
Acceptance criteria¶
- [x] Error taxonomy implemented and documented in the package README error table
- [x] Retries only on retryable classes, with a cap
- [x]
Retry-Afterhonoured - [x] No credential material in any exception message
- [x] Tests per status class via
respx
Pre-implementation notes¶
The "no credential material" criterion is already violated today, via exception chaining.
The provider raises AgentSkillsError(...) from exc, and the chained httpx exception's repr
contains the full request URL — including the query string. Any deployment passing a SAS token
or signed-URL credential through params leaks it into every traceback and every log line that
renders one. Sanitising only the message we construct is not sufficient; the cause has to be
suppressed or the URL redacted before chaining. This is a live issue, not a future one, and it
is the reason this item should be treated as security-relevant rather than purely ergonomic.
Retry must not swallow 304. _stream_bytes returns (None, headers) on a conditional-request
hit. A retry wrapper that treats "no body" as a failure would break revalidation, and 304 must
not be classified as unavailable.
Retries interact with the manifest path. A missing index.json is an expected 404 that is
translated to ResourceListingNotSupportedError; it must stay on the non-retryable branch.
Implementation notes¶
The credential leak was real and is fixed. Confirmed: raise_for_status() produces an
httpx.HTTPStatusError whose message embeds the full URL, query string included, and chaining
it put that into every rendered traceback. The cause is now suppressed with from None
throughout the request path. Two regression tests assert on
traceback.format_exception(...) output rather than on str(exc) — testing only the message
would not have caught the original bug, since the message was already clean.
Messages carry a base-relative path, not a redacted absolute URL. The first attempt
redacted the query string and kept scheme and host, which is the conventional fix. It was
abandoned because an existing security test asserts the base URL never appears in a message at
all, and that guarantee is worth keeping: the host is the one piece an operator can always
recover from configuration, whereas a leaked path in a shared log is not recoverable. Errors
now read HTTP 503 from /incident-response/SKILL.md, which is enough to debug.
401/403 are AgentSkillsError, not SkillUnavailableError. Retrying a rejected
credential is pointless and, against hosts with lockout policies, harmful. They get a message
naming headers/params as the thing to check, without echoing either.
A Retry-After beyond max_retry_delay fails immediately rather than sleeping. Honouring
a literal Retry-After: 3600 would park a request path for an hour. The advice is not
discarded — it is attached to the exception as retry_after so a scheduler can act on it — but
the decision to wait that long belongs to the caller, not to a provider.
Full jitter, not equal jitter. random.uniform(0, delay). The registry builds its catalog
concurrently, so without jitter every skill fetch retries in lockstep and hits the recovering
server as a single wave. The tests pin the bound by stubbing random.uniform, not the value.
304 stayed on the success path, as flagged above; the revalidation tests already covered it and continue to pass unchanged.
Observation. SkillUnavailableError.retry_after is currently only ever populated by the
HTTP provider. If a second provider gains transient failures, the backoff loop itself is the
next thing to lift into core — it is provider-agnostic apart from the status-code mapping.
8. Structured logging across the SDK¶
Labels: theme:operability, type:feature, package:core, package:fs, package:http
Status: implemented. Scope grew to
agentskills-mcp-server, whose existing logger sat outside the namespace.
Problem¶
Outside one warning in validation.py, the SDK is silent. There is no way to see which skills
were fetched, how long a fetch took, or why a provider fell back — which makes production
support guesswork.
Proposal¶
- Module-level loggers under a consistent
agentskills.*namespace. - Library attaches no handlers and adds
NullHandler; the host application configures output. DEBUGfor fetch/parse/cache events,INFOfor registration outcomes,WARNINGfor degraded behaviour.- A redaction helper applied to anything containing a URL, header, or query string so
Authorizationvalues and SAS tokens never reach a log record. Pairs with the error messages in issue 7.
Acceptance criteria¶
- [x] Consistent logger namespace,
NullHandlerattached - [x] No secret material in any log record (test with a provider configured with a bearer token and a SAS-style query param)
- [x] Logging conventions documented in
docs/DEVELOPMENT.md
Implementation notes¶
logging.getLogger(__name__) does not produce a namespace, and that was the actual bug. The
two existing loggers were named agentskills_core.validation and agentskills_mcp_server.config.
Those are siblings, not descendants of a common root, so
logging.getLogger("agentskills").setLevel(DEBUG) reached neither. Distribution names use
underscores; logger hierarchies use dots. get_logger(__name__) rewrites the prefix —
agentskills_http.static → agentskills.http.static — which keeps the logger name tied to the
module it lives in while giving the whole SDK one dial. Hand-written logger strings were rejected:
six packages' worth of literals rot silently when a file moves.
The NullHandler is attached once to the namespace root, not per module. The standard
library documents the per-module form, but that only exists because most libraries have no
common root. We do.
There is no ERROR level anywhere in the SDK, deliberately. Every failure path already
raises. Logging it as well reports one event twice and quietly takes the severity decision away
from the caller, who is the only party who knows whether a missing skill was fatal. The one
WARNING that is not a retry — an unrecognised metadata key — is a case where nothing is raised
at all.
Redaction reuses issue 7's work rather than inventing a second mechanism. The HTTP provider
already had a private _describe() returning a base-relative path. That is strictly stronger than
the "strip the query string" helper this issue proposed, and building a second, weaker sanitiser
for logs would have guaranteed drift between what an exception says and what a log line says.
_describe() was lifted into core as redact_url(url, relative_to=...), now tested directly, and
_describe() is a one-line call through to it.
One real bug surfaced while lifting it: the old implementation ended or url, falling back to the
raw URL when the relative path came out empty — which happens when the fetched URL equals the
base. That fallback leaked the query string in exactly the situation it was meant to protect.
redact_url() now falls back to a host-and-path form with the query replaced, never the raw
input.
Headers are not logged, redacted or otherwise. A redact_headers() helper was considered and
dropped. No operational question is answered by a header value, and a redactor that only works if
every future caller remembers to invoke it is a trap — the safe default is to have nothing to
redact. Same reasoning drove the explicit ban on exc_info for transport failures: httpx
exception reprs embed the full request URL, so _logger.warning(..., exc_info=exc) in the retry
loop would have re-opened the exact leak issue 7 closed. There is a comment at that line saying so.
The retry loop is where the real value is. Before this change a provider could burn three
attempts and several seconds of backoff with no external trace whatsoever. That is now a
WARNING naming the sanitised path, the sleep, and the attempt number.
Observations¶
agentskills_core/logging.py shadows a standard-library module name. It is safe — Python 3
absolute imports mean import logging inside the package still resolves to the stdlib — and it
matches the package's existing noun-per-module convention (encoding, parsing, validation).
Flagged because it reads like a bug at a glance and will attract at least one drive-by "fix".
The secret-leak tests assert on caplog.text, and one of them asserts the log is non-empty.
A test that greps for a token in an empty buffer passes for the wrong reason. Worth copying to
any future test of the same shape.
Timing is still not instrumented. The issue's problem statement asked how long a fetch took;
nothing here answers that. Emitting durations means either wrapping every provider call in a
timer or exposing metrics properly, and the second is the honest answer. Deferred to the
observability work rather than half-solved with time.monotonic() deltas sprinkled through the
providers.
9. Enforce a coverage floor in CI¶
Labels: theme:project-health, type:chore, good-first-issue
Status: implemented.
Problem¶
pytest-cov is not a dev dependency and CI has no coverage gate, so regressions in test
coverage are invisible.
Proposal¶
Add pytest-cov, wire python scripts/dev.py test:cov into the CI test job, set a floor
(start at current measured coverage, ratchet upward), publish the report, and add a badge.
Acceptance criteria¶
- [x] Coverage measured per package and in aggregate
- [x] CI fails below the floor
- [x] Badge in the root README
Implementation notes¶
The pre-existing test:cov task measured the wrong thing. It passed --cov=packages, a
path, which counts the test files as covered code. A test file is trivially covered by the act
of running it, so 2,822 of the 3,712 measured statements were tests marking themselves 100%.
Reported coverage was 99%; real library coverage was 96.42%. Coverage is now configured with
source_pkgs — import names, not paths — so only shipped code is measured. Branch coverage is
on, which the old configuration also lacked.
An aggregate floor alone is not a gate, and the issue's first criterion is why. With six
packages summed, agentskills-mcp-server can sit at 91% indefinitely because agentskills-core
at 99% carries the average. Floors are therefore enforced at two levels: the aggregate through
[tool.coverage.report] fail_under, so that even a bare coverage report is gated, and per
package through COVERAGE_FLOORS in scripts/dev.py, which runs coverage report --include
once per distribution against the same .coverage file. This was verified rather than assumed:
adding five uncovered statements to agentskills_core fails the per-package gate at 97% against
its floor of 99, while the aggregate does not move off 96%. An aggregate-only gate would have
passed that change.
Floors start at the measured value, truncated to an integer — core 99, fs 97, http 96, langchain 100, agentframework 100, mcp_server 91, aggregate 96. Setting them lower "for headroom" bakes in slack that nobody ever removes. Two packages sit at 100, which means any new uncovered line fails CI; for a package that is currently fully covered that is the correct signal, not an inconvenience.
Floors live in exactly one place each. The aggregate is in pyproject.toml because coverage
enforces it natively; the per-package values are in dev.py because nothing enforces those
natively. The aggregate is deliberately not repeated in dev.py — the final coverage report
call passes no --fail-under, so it reads the value from config. Two copies of a threshold is
two thresholds.
The badge states the enforced floor, not a measurement. coverage ≥96% is guaranteed true by
CI on every commit. A live per-commit percentage needs Codecov or Coveralls, which means an
account, a repository authorisation, and usually a token — a decision for the maintainer, not
something to slip into a chore PR. A hand-updated static number would have been the worst of both:
it can silently claim 96% while the truth is 80%. The floor formulation cannot drift misleadingly,
because raising the floor and updating the badge are the same commit by construction. The live
number is still visible: the CI job writes the full report to the GitHub Actions job summary of
every run, and uploads coverage.xml and htmlcov/ as an artifact.
Coverage runs as its own job, not inside the test matrix. Running it three times across 3.12/3.13/3.14 would triple the cost to answer the same question, and a version-dependent coverage difference would be a finding in its own right rather than something to average away.
Observations¶
scripts/dev.py claimed to be "Development task runner for Pallium". Copied from a sibling
repository and never corrected. Fixed in passing.
scripts/ is not in the lint scope. CI runs ruff over packages/ examples/ only, so
dev.py has drifted from the formatter — ruff format wants to rewrite lines this change did
not touch. Left alone rather than mixing a whole-file reformat into a coverage PR, but the scope
should be widened and the file formatted once, deliberately.
The gate's floor sits just below three known soft spots, which is worth naming rather than
quietly accepting. In order of value: agentskills_mcp_server/__main__.py at 75% — the
documented way to launch the server, with its config-loading error paths and the entire
_build/run path untested; agentskills_mcp_server/context_provider.py at 83%, where the
uncovered block is the import guard added by issue 11, so the two carefully worded upgrade
messages have never been executed; and the _stream_bytes size-limit branches in the HTTP
provider. These are the ratchet's first targets.
Output ordering in dev.py was scrambled. Its banners are printed by Python and buffered,
while subprocess output goes straight to the terminal, so every banner appeared after every
command's output — unreadable in a CI log with eight sequential invocations. Fixed with an
explicit flush.
10. Automate PyPI publishing with Trusted Publishing¶
Labels: theme:project-health, type:chore
Status: implemented. Requires one-time Trusted Publishing configuration on PyPI and TestPyPI (six projects each) plus two GitHub Environments before the first tag; see DEVELOPMENT.md. Until that is done, the workflow runs and fails at the upload step.
Problem¶
Releasing means a maintainer running scripts/publish.ps1 locally with PyPI credentials, in
dependency order, across six packages. It is manual, unauditable, requires a long-lived token
on a workstation, and is easy to get half-done.
Proposal¶
GitHub Actions workflow triggered by the release tag, using PyPI Trusted Publishing (OIDC, no stored token):
- Build all six packages
- Publish in dependency order:
core→ providers → integrations - Gate on a GitHub Environment requiring manual approval
- Dry-run to TestPyPI on release-candidate tags
- Attach build provenance attestations (prerequisite for artifact signing in v1.0)
Keep publish.ps1 for local/emergency use.
Acceptance criteria¶
- [x] Tag push publishes all six packages with no stored credentials
- [x] Dependency order enforced; a failure mid-sequence is clearly reported
- [ ] TestPyPI path verified end-to-end — blocked on the one-time setup below. The routing, the build loop and the version guard were exercised locally; the upload itself cannot be verified until the TestPyPI trusted publishers exist. Run the workflow manually once they do, before the first real tag.
- [x] Release process in
docs/DEVELOPMENT.mdupdated
Implementation notes¶
Tag routing allow-lists rather than deny-lists. The obvious reading of "dry-run to TestPyPI
on release-candidate tags" is to match -rc and send those to TestPyPI. That fails open: a
typo'd tag, a nightly, or a v0.3 with a missing component all match nothing and go straight
to production. The condition is inverted instead — only ^v[0-9]+\.[0-9]+\.[0-9]+$ reaches
PyPI, and everything else falls through to TestPyPI. Verified against v0.3.0, v0.3.0rc1,
v0.3.0-rc.1, v1.2.3, nightly and v0.3. The worst outcome of a bad tag is now a wasted
TestPyPI version, not a burnt PyPI one.
One approval, not six. GitHub environment reviewers approve per job, so the natural
one-job-per-package layout would have demanded five or six approvals per release. Building is
therefore split from publishing: one build job produces all six distributions in parallel, and
one publish job uploads them as six sequential steps behind a single environment gate. Sequential
steps also satisfy "a failure mid-sequence is clearly reported" better than a matrix — the step
name is the package name, and matrix ordering is not guaranteed even at max-parallel: 1.
skip-existing: true on both targets. The issue's own complaint is that a release is "easy
to get half-done"; that is only true if resuming is impossible. With skip-existing, re-running
the failed job finishes the release instead of erroring on the packages that already went up.
The failure mode this normally introduces — silently publishing nothing because the version was
never bumped — is closed by the version guard rather than by refusing to resume.
A version guard runs before anything is built. scripts/check_release_version.py reads all
six pyproject.toml files, asserts they agree, and asserts the agreed version equals the tag
minus its v. Exact string comparison rather than PEP 440 parsing, so the tag, the wheel
filename, and the PyPI page are spelled identically; the cost is that a pre-release must be
tagged v0.3.0rc1, not v0.3.0-rc.1. The script is useful on its own before tagging.
Dependency order is a user-visible property, not a PyPI constraint. PyPI will happily accept
agentskills-fs before agentskills-core exists — dependencies resolve at install time. The
order matters only for the window during a release when someone runs pip install. Recorded in
a comment, because the natural assumption is that the ordering is technically required, and
someone will otherwise "optimise" it into a parallel matrix.
Script injection was avoided in the shell steps. Tag names reach the runner as data, so
github.ref_name is passed through env: and referenced as "$REF" rather than interpolated
into the script body with ${{ }}.
Observations¶
The GitHub Release was being created whether or not publishing worked. release.yml fired
on the same tag, independently and immediately, so it would have produced a release announcing
0.3.0 even if every upload failed — and, once the approval gate existed, it would have announced
the release before the reviewer approved it. The job moved into publish.yml behind
needs: publish, and release.yml is deleted. Tags that route to TestPyPI now produce a GitHub
pre-release rather than a full one.
Six PyPI projects × two indexes = twelve trusted-publisher configurations, none of which can be created from the repository. This is the one part of the release path that stays manual, and it fails at the last step of the first release if it is skipped. Called out at the top of this issue and in DEVELOPMENT.md rather than left to be discovered.
publish.ps1 is now emergency-only and should be labelled as such in the script itself. It
is the only remaining path that needs a long-lived token, and the whole point of this change was
to stop those existing. Left in place per the proposal, but it will drift.
Found while preparing 0.3.0: the inter-package constraints would have shipped issue 11 again.
Every dependent required agentskills-core = ">=0.1.0,<1.0", and agentskills-mcp-server had no
upper bound at all. But agentskills_mcp_server/config.py imports get_logger, which this
milestone added — so pip install agentskills-mcp-server==0.3.0 could legally resolve core
0.1.0 and fail at import, exactly as the published 0.2.3 packages did. Floors raised to
>=0.3.0,<1.0; check_release_version.py now refuses to build unless every dependent requires
the core version being released, and bump-version.ps1/.sh rewrite the constraint alongside
the version so it cannot drift again. The six ship in lockstep, so exact-match is the right rule.
11. Restore compatibility with Agent Framework 1.x¶
Labels: theme:correctness, type:bug, package:agentframework, package:mcp-server
Status: implemented.
Problem¶
agent-framework-core renamed BaseContextProvider to ContextProvider. Both integrations
still import the old name:
from agent_framework import BaseContextProvider, FunctionTool
Our constraint is agent-framework-core = ">=1.0.0rc3,<2.0", which admits 1.12.1, so a fresh
pip install agentskills-agentframework resolves the new version and raises ImportError at
import time. The published 0.2.3 packages are broken for anyone installing today. CI and
local runs pass only because poetry.lock pins 1.0.0rc3.
Confirmed on a clean Python 3.14 environment: 23 failures across
agentskills-mcp-server/tests/test_context_provider.py, plus collection errors in both
agentskills-agentframework test modules. Every one traces to this rename.
A second defect compounds it. The lazy import in context_provider.py catches ImportError
and re-raises:
AgentSkillsMcpContextProvider requires agent-framework. Install it with: pip install agentskills-mcp-server[agentframework]
The package is installed. The advice is wrong, and it sends the user to reinstall a dependency they already have instead of revealing the version incompatibility.
Proposal¶
- Import
ContextProvider, falling back toBaseContextProviderso both generations work, or drop the fallback and raise the floor to the release that introduced the new name. - Decide the supported range deliberately and encode it.
<2.0was never verified against anything past the release candidate. - Re-raise the original exception as
__cause__so the real error survives, and only claim the dependency is missing when it genuinely is. - Add a CI job that resolves dependencies without the lock file, so upstream breaking changes surface here rather than in a user's install.
Acceptance criteria¶
- [x] Both integrations import cleanly against
agent-framework-core1.12.1 - [x] Full suite passes on a lock-free install
- [x] Version constraint reflects the range actually tested
- [x] The "requires agent-framework" message appears only when the package is absent
- [x] Unlocked-resolution job added to CI
Open question¶
allow-prereleases and its TODO: Remove once a stable 1.0+ is available are now obsolete —
1.12.1 is stable. Removing the flag belongs with this fix.
12. Support Python 3.14¶
Labels: theme:project-health, type:bug, package:all
Status: merged, awaiting release.
Every package pinned python = ">=3.12,<3.14", so installation failed outright on current
stable Python. The README attributed this to upstream dependencies; none of them justified it.
pyyaml, httpx, langchain-core, mcp, pydantic, pyjwt and agent-framework-core all
declare 3.14 support. The cap was inherited across all six packages, including
agentskills-core, whose only dependency is pyyaml.
Ceiling raised to <4.0 rather than removed, because langchain-core declares <4.0.0 and
Poetry requires a project range that is a subset of its dependencies. 3.14 added to the CI
matrix and the classifiers.
Verified on Python 3.14.6: all six packages install, and 265 tests pass. The only failures are issue 11 above, which is unrelated to the Python version.
The fix has no effect until a release goes out — the <3.14 cap is baked into the 0.2.3
metadata already on PyPI and cannot be changed retroactively.
Unblocked side note¶
mypy is referenced by scripts/dev.py and docs/DEVELOPMENT.md but is not installed in the
dev environment, so dev.py typecheck and dev.py check fail. Worth folding into issue 9 or
filing as a separate type:chore.