Skip to content

fix(skills): preserve unknown frontmatter keys on write, project to known keys on read #658

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rubenvdlinde wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(skills): preserve unknown frontmatter keys on write, project to known keys on read #658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b05182d
fix(skills): keep unknown frontmatter keys when a skill is overwritten
rubenvdlinde Sep 22, 2026
5dcf351
feat(skills): return only the known frontmatter keys from loadSkill
rubenvdlinde Sep 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
117 changes: 106 additions & 11 deletions lib/Service/AgentSkillsService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,20 +220,28 @@ public function storeSkill(string $userId, string $skillName, string $descriptio
$skillFolder = $skillsFolder->newFolder($skillName);
}

$skillFile = null;
if ($isOverwrite) {
$node = $skillFolder->get(self::SKILL_FILE_NAME);
if (!$node instanceof File) {
throw new NotPermittedException('SKILL.md path is not a file: ' . $skillFolder->getPath());
}
$skillFile = $node;
}

// name and description are ours to write, every other key belongs to whoever put
// it there. The union operator rather than array_merge, because YAML mapping keys
// can be integers and array_merge renumbers those.
$frontmatter = Yaml::dump([
'name' => $skillName,
'description' => $description,
]);
] + ($skillFile !== null ? $this->preservedFrontmatterKeys($skillFile) : []));
$fileContent = self::FRONTMATTER_DELIMITER . "\n"
. $frontmatter
. self::FRONTMATTER_DELIMITER . "\n\n"
. $content;

if ($isOverwrite) {
$skillFile = $skillFolder->get(self::SKILL_FILE_NAME);
if (!$skillFile instanceof File) {
throw new NotPermittedException('SKILL.md path is not a file: ' . $skillFolder->getPath());
}
if ($skillFile !== null) {
$skillFile->putContent($fileContent);
} else {
$skillFolder->newFile(self::SKILL_FILE_NAME, $fileContent);
Expand Down Expand Up @@ -292,7 +300,51 @@ private function loadSkillFromFolder(Folder $folder, string $skillName): string
if (!$skillFile instanceof File) {
throw new NotFoundException('Skill file for "' . $skillName . '" not found');
}
return $skillFile->getContent();
return $this->projectSkillDocument($skillFile);
}

/**
* Return a skill document carrying only the frontmatter keys the agent needs.
*
* The caller of loadSkill feeds the result to a language model, so every extra
* key costs tokens the model cannot spend. listSkills already answers with the
* metadata fields alone. This keeps the two ends of the API saying the same thing.
*
* A document we cannot parse is returned as it is, so a malformed skill still loads.
*/
private function projectSkillDocument(File $skillFile): string {
$content = $skillFile->getContent();

try {
[$frontmatter, $body] = $this->splitSkillDocument($content, $skillFile->getPath());
$parsed = Yaml::parse($frontmatter);
} catch (RuntimeException|ParseException $e) {
$this->logger->debug(
'Skill frontmatter could not be read, returning the document unchanged: ' . $skillFile->getPath(),
['exception' => $e]
);
return $content;
}

if (!is_array($parsed)) {
return $content;
}

$known = [];
foreach (self::FRONTMATTER_METADATA_FIELDS as $field) {
if (isset($parsed[$field])) {
$known[$field] = $parsed[$field];
}
}

if ($known === []) {
return $content;
}

return self::FRONTMATTER_DELIMITER . "\n"
. Yaml::dump($known)
. self::FRONTMATTER_DELIMITER . "\n"
. $body;
}

/**
Expand All @@ -304,22 +356,65 @@ private function loadSkillFromFolder(Folder $folder, string $skillName): string
* @throws \OCP\Lock\LockedException if the file is locked
*/
public function extractFrontmatter(File $file): string {
$content = $file->getContent();
return $this->splitSkillDocument($file->getContent(), $file->getPath())[0];
}

/**
* Split a SKILL.md document into its YAML frontmatter and the body that follows it.
*
* @param string $content the full document
* @param string $path used only to build error messages
*
* @return array{0: string, 1: string} the frontmatter, then the body
*
* @throws RuntimeException if the document has no valid frontmatter
*/
private function splitSkillDocument(string $content, string $path): array {
$delimiter = self::FRONTMATTER_DELIMITER;

// must start with the opening delimiter followed by a newline
if (!str_starts_with($content, $delimiter . "\n") && !str_starts_with($content, $delimiter . "\r\n")) {
throw new RuntimeException('Skill file missing frontmatter opening delimiter: ' . $file->getPath());
throw new RuntimeException('Skill file missing frontmatter opening delimiter: ' . $path);
}

$offset = strpos($content, "\n") + 1;
// match "---" on its own line (followed by a newline or end-of-line)
if (!preg_match('#\n' . $delimiter . '(?:\r?\n|$)#', $content, $matches, PREG_OFFSET_CAPTURE, $offset)) {
throw new RuntimeException('Skill file missing frontmatter closing delimiter: ' . $file->getPath());
throw new RuntimeException('Skill file missing frontmatter closing delimiter: ' . $path);
}
$closingPos = $matches[0][1];

return substr($content, $offset, $closingPos - $offset);
return [
substr($content, $offset, $closingPos - $offset),
substr($content, $closingPos + strlen($matches[0][0])),
];
}

/**
* Read the frontmatter keys of an existing skill file that this API does not own.
*
* Anything other than the metadata fields was put there by whoever wrote the file,
* so a store has to hand it back unchanged. A document we cannot parse has no keys
* worth keeping, and replacing it is what the caller asked for anyway.
*
* @return array<string, mixed>
*/
private function preservedFrontmatterKeys(File $skillFile): array {
try {
$parsed = Yaml::parse($this->splitSkillDocument($skillFile->getContent(), $skillFile->getPath())[0]);
} catch (RuntimeException|ParseException $e) {
$this->logger->debug(
'Skill frontmatter could not be read, storing without preserved keys: ' . $skillFile->getPath(),
['exception' => $e]
);
return [];
}

if (!is_array($parsed)) {
return [];
}

return array_diff_key($parsed, array_flip(self::FRONTMATTER_METADATA_FIELDS));
}

/**
Expand Down
99 changes: 99 additions & 0 deletions tests/unit/Service/AgentSkillsServiceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,17 @@ private function writeRawSkillFile(string $uid, string $skillName, string $rawCo
return $node->newFile('SKILL.md', $rawContent);
}

/**
* Read a SKILL.md straight off disk, so a test can see what was stored rather
* than what the API chooses to return.
*/
private function readRawSkillFile(string $uid, string $skillName): string {
$userFolder = $this->rootFolder->getUserFolder($uid);
/** @var File $file */
$file = $userFolder->get($this->getSkillsPath($uid) . '/' . $skillName . '/SKILL.md');
return $file->getContent();
}

// -------------------------------------------------------------------------
// extractFrontmatter – real File node
// -------------------------------------------------------------------------
Expand Down Expand Up @@ -286,6 +297,60 @@ public function testStoreSkillOverwritesExistingSkill(): void {
$this->assertSame('overwritten', $result);
}

public function testStoreSkillPreservesUnknownFrontmatterKeys(): void {
$raw = "---\nname: my-skill\ndescription: First\nmaturityLevel: 3\nstate: active\n"
. "levelEvidence:\n - eval-run-221\n---\n\nFirst body";
$this->writeRawSkillFile(self::TEST_USER, 'my-skill', $raw);

$this->service->storeSkill(self::TEST_USER, 'my-skill', 'Second', 'Second body');

$stored = $this->readRawSkillFile(self::TEST_USER, 'my-skill');

$this->assertStringContainsString('name: my-skill', $stored);
$this->assertStringContainsString('description: Second', $stored);
$this->assertStringContainsString('maturityLevel: 3', $stored);
$this->assertStringContainsString('state: active', $stored);
$this->assertStringContainsString('eval-run-221', $stored);
$this->assertStringContainsString('Second body', $stored);
$this->assertStringNotContainsString('First body', $stored);
}

public function testStoreSkillPreservesNumericFrontmatterKeys(): void {
// YAML mapping keys can be integers, and array_merge would renumber them
$raw = "---\nname: my-skill\ndescription: First\n2024: kept\n---\n\nBody";
$this->writeRawSkillFile(self::TEST_USER, 'my-skill', $raw);

$this->service->storeSkill(self::TEST_USER, 'my-skill', 'Second', 'Body');

$stored = $this->readRawSkillFile(self::TEST_USER, 'my-skill');

$this->assertStringContainsString('2024: kept', $stored);
$this->assertStringNotContainsString('0: kept', $stored);
}

public function testStoreSkillKeepsNameAndDescriptionFirst(): void {
$raw = "---\nname: my-skill\ndescription: First\nzzzLast: kept\n---\n\nBody";
$this->writeRawSkillFile(self::TEST_USER, 'my-skill', $raw);

$this->service->storeSkill(self::TEST_USER, 'my-skill', 'Second', 'Body');

$stored = $this->readRawSkillFile(self::TEST_USER, 'my-skill');

$this->assertStringStartsWith("---\nname: my-skill\ndescription: Second\n", $stored);
}

public function testStoreSkillOverwritesUnparsableFrontmatter(): void {
$this->writeRawSkillFile(self::TEST_USER, 'my-skill', "no frontmatter at all\n");

$result = $this->service->storeSkill(self::TEST_USER, 'my-skill', 'Rebuilt', 'Fresh body');

$stored = $this->readRawSkillFile(self::TEST_USER, 'my-skill');

$this->assertSame('overwritten', $result);
$this->assertStringStartsWith("---\nname: my-skill\ndescription: Rebuilt\n---\n", $stored);
$this->assertStringContainsString('Fresh body', $stored);
}

public function testStoreSkillRejectsEmptyName(): void {
$this->expectException(\InvalidArgumentException::class);
$this->service->storeSkill(self::TEST_USER, '', 'desc', 'body');
Expand Down Expand Up @@ -371,6 +436,40 @@ public function testLoadSkillReturnsMultilineContent(): void {
$this->assertStringContainsString('- item two', $content);
}

public function testLoadSkillReturnsOnlyKnownFrontmatterKeys(): void {
$raw = "---\nname: my-skill\ndescription: Does something\nmaturityLevel: 3\nstate: active\n"
. "levelEvidence:\n - eval-run-221\n---\n\n## Instructions here";
$this->writeRawSkillFile(self::TEST_USER, 'my-skill', $raw);

$content = $this->service->loadSkill(self::TEST_USER, 'my-skill');

// the dumper quotes multi-word values, so assert the value and not its quoting
$this->assertStringContainsString('name: my-skill', $content);
$this->assertStringContainsString('Does something', $content);
$this->assertStringContainsString('## Instructions here', $content);
$this->assertStringNotContainsString('maturityLevel', $content);
$this->assertStringNotContainsString('state: active', $content);
$this->assertStringNotContainsString('eval-run-221', $content);
}

public function testLoadSkillKeepsTheBodyByteForByte(): void {
$body = "## Step 1\n\nDo the first thing.\n\n---\n\nA rule line in the body.\n";
$raw = "---\nname: my-skill\ndescription: Does something\nextra: dropped\n---\n\n" . $body;
$this->writeRawSkillFile(self::TEST_USER, 'my-skill', $raw);

$content = $this->service->loadSkill(self::TEST_USER, 'my-skill');

$this->assertStringEndsWith("\n\n" . $body, $content);
$this->assertStringNotContainsString('extra: dropped', $content);
}

public function testLoadSkillReturnsUnparsableDocumentUnchanged(): void {
$raw = "no frontmatter at all\n\n## Body";
$this->writeRawSkillFile(self::TEST_USER, 'my-skill', $raw);

$this->assertSame($raw, $this->service->loadSkill(self::TEST_USER, 'my-skill'));
}

public function testLoadSkillThrowsForMissingSkill(): void {
$this->expectException(\OCP\Files\NotFoundException::class);
$this->service->loadSkill(self::TEST_USER, 'non-existent-skill');
Expand Down