diff --git a/lib/Service/AgentSkillsService.php b/lib/Service/AgentSkillsService.php index 60009aabe..9c308f6e4 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); @@ -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; } /** @@ -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 + */ + 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..1eb1c45c2 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'); @@ -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');