feat: refactor SkillsSynchronizer and SkillsCommand for improved logging and synchronization handling

coisa · coisa · commit 5e24965ed129 · 2026-04-09T21:18:12.000-03:00
Signed-off-by: Felipe Sayão Lobato Abreu &lt;github@mentordosnerds.com&gt;
diff --git a/src/Agent/Skills/SkillsSynchronizer.php b/src/Agent/Skills/SkillsSynchronizer.php
@@ -18,7 +18,9 @@
 
 namespace FastForward\DevTools\Agent\Skills;
 
-use Psr\Log\LoggerInterface;
+use Psr\Log\LoggerAwareInterface;
+use Psr\Log\LoggerAwareTrait;
+use Psr\Log\NullLogger;
 use Symfony\Component\Filesystem\Filesystem;
 use Symfony\Component\Finder\Finder;
 use Symfony\Component\Filesystem\Path;
@@ -30,16 +32,23 @@
  * repository to the skills packaged within the fast-forward/dev-tools dependency.
  * It handles initial sync, idempotent re-runs, and cleanup of broken links.
  */
-final class SkillsSynchronizer
+final class SkillsSynchronizer implements LoggerAwareInterface
 {
-    private readonly Filesystem $filesystem;
+    use LoggerAwareTrait;
 
     /**
+     * Initializes the synchronizer with an optional filesystem instance.
+     *
+     * If no filesystem is provided, a default {@see Filesystem} instance is created.
+     *
      * @param Filesystem|null $filesystem Filesystem instance for file operations
+     * @param Finder $finder Finder instance for locating skill directories in the package
      */
-    public function __construct(?Filesystem $filesystem = null)
-    {
-        $this->filesystem = $filesystem ?? new Filesystem();
+    public function __construct(
+        private readonly Filesystem $filesystem = new Filesystem(),
+        private readonly Finder $finder = new Finder(),
+    ) {
+        $this->logger = new NullLogger();
     }
 
     /**
@@ -51,30 +60,26 @@ public function __construct(?Filesystem $filesystem = null)
      *
      * @param string $skillsDir Absolute path to the consumer's .agents/skills directory
      * @param string $packageSkillsPath Absolute path to the packaged skills in the dependency
-     * @param LoggerInterface $logger Logger for reporting sync operations
      *
      * @return SynchronizeResult Result containing counts of created, preserved, and removed links
      */
-    public function synchronize(
-        string $skillsDir,
-        string $packageSkillsPath,
-        LoggerInterface $logger,
-    ): SynchronizeResult {
+    public function synchronize(string $skillsDir, string $packageSkillsPath): SynchronizeResult
+    {
         $result = new SynchronizeResult();
 
         if (! $this->filesystem->exists($packageSkillsPath)) {
-            $logger->error('No packaged skills found at: ' . $packageSkillsPath);
+            $this->logger->error('No packaged skills found at: ' . $packageSkillsPath);
             $result->markFailed();
 
             return $result;
         }
 
         if (! $this->filesystem->exists($skillsDir)) {
             $this->filesystem->mkdir($skillsDir);
-            $logger->info('Created .agents/skills directory.');
+            $this->logger->info('Created .agents/skills directory.');
         }
 
-        $this->syncPackageSkills($skillsDir, $packageSkillsPath, $logger, $result);
+        $this->syncPackageSkills($skillsDir, $packageSkillsPath, $result);
 
         return $result;
     }
@@ -87,16 +92,14 @@ public function synchronize(
      *
      * @param string $skillsDir Target directory for symlinks
      * @param string $packageSkillsPath Source directory containing packaged skills
-     * @param LoggerInterface $logger Logger for operation feedback
      * @param SynchronizeResult $result Result object to track outcomes
      */
     private function syncPackageSkills(
         string $skillsDir,
         string $packageSkillsPath,
-        LoggerInterface $logger,
         SynchronizeResult $result,
     ): void {
-        $finder = Finder::create()
+        $finder = $this->finder
             ->directories()
             ->in($packageSkillsPath)
             ->depth('== 0');
@@ -106,7 +109,7 @@ private function syncPackageSkills(
             $targetLink = Path::makeAbsolute($skillName, $skillsDir);
             $sourcePath = $skillDir->getRealPath();
 
-            $this->processSkillLink($skillName, $targetLink, $sourcePath, $logger, $result);
+            $this->processSkillLink($skillName, $targetLink, $sourcePath, $result);
         }
     }
 
@@ -119,29 +122,27 @@ private function syncPackageSkills(
      * @param string $skillName Name of the skill being processed
      * @param string $targetLink Absolute path where the symlink should exist
      * @param string $sourcePath Absolute path to the packaged skill directory
-     * @param LoggerInterface $logger Logger for feedback on actions taken
      * @param SynchronizeResult $result Result tracker for reporting outcomes
      */
     private function processSkillLink(
         string $skillName,
         string $targetLink,
         string $sourcePath,
-        LoggerInterface $logger,
         SynchronizeResult $result,
     ): void {
         if (! $this->filesystem->exists($targetLink)) {
-            $this->createNewLink($skillName, $targetLink, $sourcePath, $logger, $result);
+            $this->createNewLink($skillName, $targetLink, $sourcePath, $result);
 
             return;
         }
 
         if (! $this->isSymlink($targetLink)) {
-            $this->preserveExistingNonSymlink($skillName, $logger, $result);
+            $this->preserveExistingNonSymlink($skillName, $result);
 
             return;
         }
 
-        $this->processExistingSymlink($skillName, $targetLink, $sourcePath, $logger, $result);
+        $this->processExistingSymlink($skillName, $targetLink, $sourcePath, $result);
     }
 
     /**
@@ -153,18 +154,16 @@ private function processSkillLink(
      * @param string $skillName Name identifying the skill
      * @param string $targetLink Absolute path where the symlink will be created
      * @param string $sourcePath Absolute path to the packaged skill directory
-     * @param LoggerInterface $logger Logger for confirmation message
      * @param SynchronizeResult $result Result object for tracking creation
      */
     private function createNewLink(
         string $skillName,
         string $targetLink,
         string $sourcePath,
-        LoggerInterface $logger,
         SynchronizeResult $result,
     ): void {
         $this->filesystem->symlink($sourcePath, $targetLink);
-        $logger->info('Created link: ' . $skillName . ' -> ' . $sourcePath);
+        $this->logger->info('Created link: ' . $skillName . ' -> ' . $sourcePath);
         $result->addCreatedLink($skillName);
     }
 
@@ -176,15 +175,11 @@ private function createNewLink(
      * replaced to avoid accidental data loss.
      *
      * @param string $skillName Name of the skill with the conflicting item
-     * @param LoggerInterface $logger Logger for the preservation notice
      * @param SynchronizeResult $result Result tracker for preserved items
      */
-    private function preserveExistingNonSymlink(
-        string $skillName,
-        LoggerInterface $logger,
-        SynchronizeResult $result,
-    ): void {
-        $logger->notice('Existing non-symlink found: ' . $skillName . ' (keeping as is, skipping link creation)');
+    private function preserveExistingNonSymlink(string $skillName, SynchronizeResult $result): void
+    {
+        $this->logger->notice('Existing non-symlink found: ' . $skillName . ' (keeping as is, skipping link creation)');
         $result->addPreservedLink($skillName);
     }
 
@@ -197,25 +192,23 @@ private function preserveExistingNonSymlink(
      * @param string $skillName Name of the skill with the existing symlink
      * @param string $targetLink Absolute path to the existing symlink
      * @param string $sourcePath Absolute path to the expected source directory
-     * @param LoggerInterface $logger Logger for preservation or repair messages
      * @param SynchronizeResult $result Result tracker for preserved or removed links
      */
     private function processExistingSymlink(
         string $skillName,
         string $targetLink,
         string $sourcePath,
-        LoggerInterface $logger,
         SynchronizeResult $result,
     ): void {
         $linkPath = $this->filesystem->readlink($targetLink, true);
 
         if (! $linkPath || ! $this->filesystem->exists($linkPath)) {
-            $this->repairBrokenLink($skillName, $targetLink, $sourcePath, $logger, $result);
+            $this->repairBrokenLink($skillName, $targetLink, $sourcePath, $result);
 
             return;
         }
 
-        $logger->notice('Preserved existing link: ' . $skillName);
+        $this->logger->notice('Preserved existing link: ' . $skillName);
         $result->addPreservedLink($skillName);
     }
 
@@ -229,21 +222,19 @@ private function processExistingSymlink(
      * @param string $skillName Name of the skill with the broken symlink
      * @param string $targetLink Absolute path to the broken symlink
      * @param string $sourcePath Absolute path to the current packaged skill
-     * @param LoggerInterface $logger Logger for repair and creation messages
      * @param SynchronizeResult $result Result tracker for removed and created items
      */
     private function repairBrokenLink(
         string $skillName,
         string $targetLink,
         string $sourcePath,
-        LoggerInterface $logger,
         SynchronizeResult $result,
     ): void {
         $this->filesystem->remove($targetLink);
-        $logger->notice('Existing link is broken: ' . $skillName . ' (removing and recreating)');
+        $this->logger->notice('Existing link is broken: ' . $skillName . ' (removing and recreating)');
         $result->addRemovedBrokenLink($skillName);
 
-        $this->createNewLink($skillName, $targetLink, $sourcePath, $logger, $result);
+        $this->createNewLink($skillName, $targetLink, $sourcePath, $result);
     }
 
     /**
@@ -253,6 +244,6 @@ private function repairBrokenLink(
      */
     private function isSymlink(string $path): bool
     {
-        return is_link($path);
+        return null !== $this->filesystem->readlink($path);
     }
 }
diff --git a/src/Agent/Skills/SynchronizeResult.php b/src/Agent/Skills/SynchronizeResult.php
@@ -48,6 +48,9 @@ final class SynchronizeResult
      */
     private array $removedBrokenLinks = [];
 
+    /**
+     * Indicates whether the synchronization encountered a failure.
+     */
     private bool $failed = false;
 
     /**
diff --git a/src/Command/SkillsCommand.php b/src/Command/SkillsCommand.php
@@ -22,25 +22,53 @@
 use FastForward\DevTools\Agent\Skills\SynchronizeResult;
 use Symfony\Component\Console\Input\InputInterface;
 use Symfony\Component\Console\Output\OutputInterface;
+use Symfony\Component\Filesystem\Filesystem;
 
 /**
- * Synchronizes Fast Forward skills into the consumer repository by managing `.agents/skills` links.
+ * Synchronizes packaged Fast Forward skills into the consumer repository.
+ *
+ * This command SHALL ensure that the consumer repository contains the expected
+ * `.agents/skills` directory structure backed by the packaged skill set. The
+ * command MUST verify that the packaged skills directory exists before any
+ * synchronization is attempted. If the target skills directory does not exist,
+ * it SHALL be created before the synchronization process begins.
+ *
+ * The synchronization workflow is delegated to {@see SkillsSynchronizer}. This
+ * command MUST act as an orchestration layer only: it prepares the source and
+ * target paths, triggers synchronization, and translates the resulting status
+ * into Symfony Console output and process exit codes.
  */
 final class SkillsCommand extends AbstractCommand
 {
-    private readonly SkillsSynchronizer $synchronizer;
-
     /**
-     * @param SkillsSynchronizer|null $synchronizer
+     * Initializes the command with an optional skills synchronizer instance.
+     *
+     * If no synchronizer is provided, the command SHALL instantiate the default
+     * {@see SkillsSynchronizer} implementation. Consumers MAY inject a custom
+     * synchronizer for testing or alternative synchronization behavior, provided
+     * it preserves the expected contract.
+     *
+     * @param SkillsSynchronizer|null $synchronizer the synchronizer responsible
+     *                                              for applying the skills
+     *                                              synchronization process
+     * @param Filesystem|null $filesystem filesystem used to resolve
+     *                                    and manage the skills
+     *                                    directory structure
      */
-    public function __construct(?SkillsSynchronizer $synchronizer = null)
-    {
-        $this->synchronizer = $synchronizer ?? new SkillsSynchronizer();
-
-        parent::__construct();
+    public function __construct(
+        private readonly SkillsSynchronizer $synchronizer = new SkillsSynchronizer(),
+        ?Filesystem $filesystem = null
+    ) {
+        parent::__construct($filesystem);
     }
 
     /**
+     * Configures the command name, description, and help text.
+     *
+     * The command metadata MUST clearly describe that the operation synchronizes
+     * Fast Forward skills into the `.agents/skills` directory and that it manages
+     * link-based synchronization for packaged skills.
+     *
      * @return void
      */
     protected function configure(): void
@@ -55,10 +83,25 @@ protected function configure(): void
     }
 
     /**
-     * @param InputInterface $input
-     * @param OutputInterface $output
+     * Executes the skills synchronization workflow.
+     *
+     * This method SHALL:
+     * - announce the start of synchronization;
+     * - resolve the packaged skills path and consumer target directory;
+     * - fail when the packaged skills directory does not exist;
+     * - create the target directory when it is missing;
+     * - delegate synchronization to {@see SkillsSynchronizer};
+     * - return a success or failure exit code based on the synchronization result.
      *
-     * @return int
+     * The command MUST return {@see self::FAILURE} when packaged skills are not
+     * available or when the synchronizer reports a failure. It MUST return
+     * {@see self::SUCCESS} only when synchronization completes successfully.
+     *
+     * @param InputInterface $input the console input instance provided by Symfony
+     * @param OutputInterface $output the console output instance used to report progress
+     *
+     * @return int The process exit status. This MUST be {@see self::SUCCESS} on
+     *             success and {@see self::FAILURE} on failure.
      */
     protected function execute(InputInterface $input, OutputInterface $output): int
     {
@@ -67,7 +110,6 @@ protected function execute(InputInterface $input, OutputInterface $output): int
         $packageSkillsPath = $this->getDevToolsFile('.agents/skills');
         $skillsDir = $this->getConfigFile('.agents/skills', true);
 
-        // Normal consumer repository flow
         if (! $this->filesystem->exists($packageSkillsPath)) {
             $output->writeln('<comment>No packaged skills found at: ' . $packageSkillsPath . '</comment>');
 
@@ -79,8 +121,10 @@ protected function execute(InputInterface $input, OutputInterface $output): int
             $output->writeln('<info>Created .agents/skills directory.</info>');
         }
 
+        $this->synchronizer->setLogger($this->getIO());
+
         /** @var SynchronizeResult $result */
-        $result = $this->synchronizer->synchronize($skillsDir, $packageSkillsPath, $this->getIO());
+        $result = $this->synchronizer->synchronize($skillsDir, $packageSkillsPath);
 
         if ($result->failed()) {
             $output->writeln('<error>Skills synchronization failed.</error>');
