Repository navigation
Serve a placeholder agent skill - #2625
Conversation
|
🔎 A preview deployment of this pull request is available at https://utilities.philanthropydatacommons.org/pdc-service-preview-2625-bxa2y/ (and upstream https://pdc-service-preview-2625-bxa2y.ondigitalocean.app) assuming this PR is still open. 🔍 |
There was a problem hiding this comment.
🔵 Needs a closer look
The generated skill URL must be dereferenceable via the canonical well-known path or a fully qualified URL.
0 open findings
What changed in this PR
Adds public Agent Skills Discovery endpoints and serves a static placeholder PDC skill.
Changes:
- Generates a skill index with SHA-256 digests.
- Serves
SKILL.mdthrough unauthenticated routes. - Adds tests, packaging updates, dependency changes, and documentation.
Review note: The generated skill URL is relative and should use the canonical well-known path or a fully qualified URL.
| File | Reviewed changes |
|---|---|
src/skills/philanthropy-data-commons/SKILL.md |
Placeholder skill content |
src/routers/index.ts |
Registers discovery routes |
src/routers/agentSkillsRouter.ts |
Serves public skill routes |
src/handlers/agentSkillsHandlers.ts |
Handles skill index responses |
src/agentSkills.ts |
Parses metadata and generates digests |
src/__tests__/agentSkills.int.test.ts |
Adds endpoint and digest coverage |
package.json |
Adds YAML dependency and asset packaging |
package-lock.json |
Locks dependency updates |
CHANGELOG.md |
Documents new endpoints |
AGENTS.md |
Documents skill conventions |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2625 +/- ##
==========================================
- Coverage 94.06% 94.03% -0.03%
==========================================
Files 346 348 +2
Lines 4462 4492 +30
Branches 588 588
==========================================
+ Hits 4197 4224 +27
- Misses 264 267 +3
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
d27d5f0 to
5b54445
Compare
| description: string; | ||
| } | ||
|
|
||
| const isSkillFrontmatter = (value: unknown): value is SkillFrontmatter => |
There was a problem hiding this comment.
This is kind of a linter, which is out of place for a test IMHO. I did explore whether linters for skills exist and they do, but it's early days and they would likely become dead dependencies. Given that, I figured keeping this for now was a reasonable tradeoff.
bickelj-agent
left a comment
bickelj-agent
left a comment
There was a problem hiding this comment.
Two notes below. The relative url in index.json is fine — the Agent Skills Discovery RFC explicitly permits relative URLs resolved against the index URL (RFC 3986 §5) and gives code-review/SKILL.md as an example, so the index is RFC-compliant as written.
— GLM-5.2
bickelj
left a comment
bickelj
left a comment
There was a problem hiding this comment.
Given that we already have static assets being served, I definitely like the approach of simply serving the skills rather than trying to integrate S3 somehow.
Something doesn't work right with the preview environment, though. See https://utilities.philanthropydatacommons.org/pdc-service-preview-2625-7t787/.well-known/agent-skills/
I think maybe something is set as absolute where it should be relative or vice versa. Also see GLM-5.2's feedback (take what's useful and leave what's not).
a138b0a to
51abf27
Compare
|
The issue is that static files redirect if you request a directory, and the redirect apparently doesn't account for the preview's path prefix. If you go to https://utilities.philanthropydatacommons.org/pdc-service-preview-2625-3pie8/.well-known/agent-skills/index.json it works (but I also changed the redirect). |
bickelj-agent
left a comment
bickelj-agent
left a comment
There was a problem hiding this comment.
Re-reviewed after the force-push to 51abf27. Both prior comments are addressed: the index Promise cache now resets to null on rejection so a later request can recover, and the index test sorts directory names before comparing. The new { redirect: false } on the static middleware plus the 404-on-directory test is a nice hardening; tsc/eslint/prettier are clean and I have no further comments.
— GLM-5.2
bickelj
left a comment
bickelj
left a comment
There was a problem hiding this comment.
This looks very close to done.
I see a few places that need allNoLeaks rather than Promise.all.
This URL is also a mouthful or contextful: https://utilities.philanthropydatacommons.org/pdc-service-preview-2625-3kq3c/.well-known/agent-skills/philanthropy-data-commons/SKILL.md
Since the words Philanthropy Data Commons are usually/always in our domain names, perhaps the skills could be under .well-known/agent-skills/pdc or .well-known/agent-skills/pdc-service or something like that, saving a handful of tokens when reading the index.json and visiting the URL. Not a dealbreaker though, just a thought.
51abf27 to
2a758b8
Compare
Agentic tools need a predictable place to find instructions for working with the PDC. The skill is a static file shipped with the service, so it can never be generated from user data and always matches the code that serves it. Issue #2555 Create a new public endpoint to provide an LLM skill Claude-Session: 849da041-1b51-45be-9b62-8042b2cd31bb
2a758b8 to
9bb34d3
Compare
This PR adds a pdc skill to our service API via
.well-known(which is the industry standard)I had been thinking that there would be some kind of bundle + S3 approach... but as I implemented it it became clear that was (A) a lot of complexity for little upside and worse (B) it coupled published artifacts with a runtime routes in an awkward way.
This approach just serves the file as a static file from the server itself. We may get more complex with the skill and need a bundle step but even then the bundle can be performed on server start and can either upload to S3 or serve directly depending on the total skill size.
Resolves #2555