From b05182de4c75de5f94027c6834fcdd36ec938f02 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Tue, 22 Sep 2026 18:21:58 +0200 Subject: [PATCH 1/2] fix(skills): keep unknown frontmatter keys when a skill is overwritten 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 --- lib/Service/AgentSkillsService.php | 71 ++++++++++++++++--- tests/unit/Service/AgentSkillsServiceTest.php | 65 +++++++++++++++++ 2 files changed, 126 insertions(+), 10 deletions(-) diff --git a/lib/Service/AgentSkillsService.php b/lib/Service/AgentSkillsService.php index 60009aabe..39af2922f 100644 --- a/lib/Service/AgentSkillsService.php +++ b/lib/Service/AgentSkillsService.php @@ -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); @@ -304,22 +312,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 + */ + 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)); } /** diff --git a/tests/unit/Service/AgentSkillsServiceTest.php b/tests/unit/Service/AgentSkillsServiceTest.php index 1b8cde39e..ef7358f56 100644 --- a/tests/unit/Service/AgentSkillsServiceTest.php +++ b/tests/unit/Service/AgentSkillsServiceTest.php @@ -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 // ------------------------------------------------------------------------- @@ -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'); From 5dcf351b6b7832c054296fdb6560bf6913dfb125 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Tue, 22 Sep 2026 18:22:06 +0200 Subject: [PATCH 2/2] feat(skills): return only the known frontmatter keys from loadSkill 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 --- lib/Service/AgentSkillsService.php | 46 ++++++++++++++++++- tests/unit/Service/AgentSkillsServiceTest.php | 34 ++++++++++++++ 2 files changed, 79 insertions(+), 1 deletion(-) diff --git a/lib/Service/AgentSkillsService.php b/lib/Service/AgentSkillsService.php index 39af2922f..9c308f6e4 100644 --- a/lib/Service/AgentSkillsService.php +++ b/lib/Service/AgentSkillsService.php @@ -300,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; } /** diff --git a/tests/unit/Service/AgentSkillsServiceTest.php b/tests/unit/Service/AgentSkillsServiceTest.php index ef7358f56..1eb1c45c2 100644 --- a/tests/unit/Service/AgentSkillsServiceTest.php +++ b/tests/unit/Service/AgentSkillsServiceTest.php @@ -436,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');