fix(skills): preserve unknown frontmatter keys on write, project to known keys on read - #658
rubenvdlinde wants to merge 2 commits into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The metadata overwrite and load behavior has no remaining actionable risk from the reviewed changes. The numeric-key preservation regression is addressed, and empty frontmatter safely follows the existing fallback path. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: be531ec9-9afe-4fd6-abfe-8baa965a5283
📒 Files selected for processing (2)
lib/Service/AgentSkillsService.phptests/unit/Service/AgentSkillsServiceTest.php
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
|
|
||
| // name and description are ours to write. Every other key belongs to whoever | ||
| // put it there, so an overwrite keeps it instead of dropping it silently. | ||
| $frontmatter = Yaml::dump(array_merge( |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Preserve numeric YAML keys during overwrite.
Yaml::parse() can produce integer mapping keys. array_merge() renumbers those keys, so an existing key such as 2024: is stored as 0: after an overwrite. Use the array union operator after filtering the owned keys. Add a test with a numeric custom frontmatter key.
Proposed fix
- $frontmatter = Yaml::dump(array_merge(
- [
+ $frontmatter = Yaml::dump(
+ [
'name' => $skillName,
'description' => $description,
- ],
- $skillFile !== null ? $this->preservedFrontmatterKeys($skillFile) : [],
- ));
+ ] + ($skillFile !== null ? $this->preservedFrontmatterKeys($skillFile) : []),
+ );
storeSkill rebuilt the frontmatter from name and description and wrote it over whatever was there, so any other key was destroyed and nothing reported it. A store_skill tool call was enough to lose metadata a user or another app had written. It now reads the existing frontmatter first and keeps every key it does not own. name and description stay first in the block. Keys are combined with the union operator rather than array_merge, because YAML mapping keys can be integers and array_merge renumbers those. A file whose frontmatter cannot be parsed is replaced as before, since that is what the caller asked for, and the reason is logged at debug level. extractFrontmatter moves onto a shared splitter that also returns the body. Its signature, its two exception messages and its tests are unchanged. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Ruben van der Linde <ruben@conduction.nl>
loadSkillFromFolder returned the file as it was, so every frontmatter key reached the caller. That caller is context_agent's load_skill, which hands the string straight to a language model, so each extra key costs tokens the model cannot spend. loadSkill now returns the body plus name and description, which is the contract listSkills already follows. A document without valid frontmatter is returned unchanged, so a malformed skill still loads. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Ruben van der Linde <ruben@conduction.nl>
90a4171 to
5dcf351
Compare
1 participant
Implements #657.
listSkillsalready answers withnameanddescriptionalone.loadSkillreturns the raw file andstoreSkillrewrites the frontmatter from scratch. This makes all three agree: the file keeps every key, the API hands back the ones the agent needs.Two commits, and the first stands on its own if the second needs more discussion.
1. Keep unknown frontmatter keys when a skill is overwritten
storeSkillbuilt the frontmatter fromnameanddescriptionand wrote it over whatever was there. Any other key was destroyed, and nothing reported it. Astore_skilltool call was enough to lose metadata a user or another app had written.It now reads the existing frontmatter first and keeps everything it does not own.
nameanddescriptionstay first in the block. A file whose frontmatter cannot be parsed is replaced as before, since that is what the caller asked for, and the reason is logged at debug level.2. Return only the known frontmatter keys from
loadSkillloadSkillFromFolderended inreturn $skillFile->getContent();, so every key reached the caller. The caller is context_agent'sload_skill, which passes the string straight to a model, so each extra key costs tokens the model cannot spend. nextcloud/context_agent#212 measures the same problem on the list path.loadSkillnow returns the body plusnameanddescription. A document without valid frontmatter is returned unchanged, so a malformed skill still loads.What changes for callers
The response of
GET /ocs/v2.php/apps/assistant/api/v1/skills/{skillName}no longer carries frontmatter keys beyondnameanddescription. The body is byte for byte what it was.This is the contract
listSkillsalready follows, and context_agent is the only consumer we know of. Say the word if you would rather have this behind a parameter and I will add one.Testing
Six tests added to
AgentSkillsServiceTest, three per commit, covering preservation on overwrite, key ordering, an unparsable existing file, projection on load, a body that contains its own---rule line, and an unparsable document loading unchanged.AgentSkillsServiceTestruns green against a Nextcloud 35.0.1 RC1 instance:OK (37 tests, 69 assertions).extractFrontmatteris refactored onto a shared splitter that also returns the body. Its signature, its two exception messages and its existing tests are untouched.🤖 AI (if applicable)