Allow fully qualified paths for new secret name in secrets rename

src/secrets/CommandRename.ts

@@ -20,13 +20,25 @@ class CommandRename extends CommandPolykey {
     this.addOption(binOptions.nodeId);
     this.addOption(binOptions.clientHost);
     this.addOption(binOptions.clientPort);
-    this.action(async (secretPath, newSecretName, options) => {
+    this.action(async (secretPath, newSecretNameArg, options) => {
       // Ensure that a valid secret path is provided
-      if (secretPath[1] == null) {
+      if (
+        secretPath[1] == null ||
+        secretPath[1].trim() === '' ||
+        secretPath[1].trim() === '/'
+      ) {
         throw new errors.ErrorPolykeyCLIRenameSecret(
           'EPERM: Cannot rename vault root',
         );
       }
+
+      // Process the newSecretNameArg using the abstracted helper method.
+      // secretPath[0] is the original vault name.
+      const finalNewSecretName = CommandRename._processNewSecretNameArgument(
+        newSecretNameArg,
+        secretPath[0],
+      );
+
       const { default: PolykeyClient } = await import(
         'polykey/PolykeyClient.js'
       );
@@ -63,7 +75,7 @@ class CommandRename extends CommandPolykey {
               metadata: auth,
               nameOrId: secretPath[0],
               secretName: secretPath[1],
-              newSecretName: newSecretName,
+              newSecretName: finalNewSecretName,
             }),
           meta,
         );
@@ -72,6 +84,67 @@ class CommandRename extends CommandPolykey {
       }
     });
   }
+
+  /**
+   * Processes the newSecretName argument to ensure it's a valid base name.
+   */
+  private static _processNewSecretNameArgument(
+    newSecretNameArg: string,
+    originalVaultName: string,
+  ): string {
+    let finalNewBaseName = newSecretNameArg;
+
+    if (newSecretNameArg.includes(':')) {
+      let parsedNewPath: Array<any>;
+      try {
+        parsedNewPath = binParsers.parseSecretPath(newSecretNameArg);
+      } catch (e: any) {
+        throw new errors.ErrorPolykeyCLIRenameSecret(
+          `EINVALID: The new secret name '${newSecretNameArg}' appears to be a path but could not be parsed: ${e.message}`,
+        );
+      }
+      const newVaultNameInArg: string = parsedNewPath[0];
+      const newSecretPathPart: string = parsedNewPath[1];
+      if (newVaultNameInArg !== originalVaultName) {
+        throw new errors.ErrorPolykeyCLIRenameSecret(
+          `ECROSSVAULT: Renaming to a different vault ('${newVaultNameInArg}') is not supported by this command. The target vault must be the same as the source vault ('${originalVaultName}').`,
+        );
+      }
+      if (
+        newSecretPathPart == null ||
+        newSecretPathPart.trim() === '' ||
+        newSecretPathPart.trim() === '/'
+      ) {
+        throw new errors.ErrorPolykeyCLIRenameSecret(
+          `EINVALID: The path component of the new secret name '${newSecretNameArg}' is empty or invalid.`,
+        );
+      }
+      const parts = newSecretPathPart.split('/').filter((p) => p.length > 0);
+      if (parts.length === 0) {
+        throw new errors.ErrorPolykeyCLIRenameSecret(
+          `EINVALID: Could not extract a valid base name from the path component '${newSecretPathPart}' in '${newSecretNameArg}'.`,
+        );
+      }
+      finalNewBaseName = parts[parts.length - 1];
+    } else {
+      if (newSecretNameArg.includes('/')) {
+        throw new errors.ErrorPolykeyCLIRenameSecret(
+          `EINVALIDNAME: The new secret name '${newSecretNameArg}' must be a base name and cannot contain '/'. If you intended to specify a path, include the vault name (e.g., '${originalVaultName}:/path/NewName').`,
+        );
+      }
+    }
+    if (!finalNewBaseName || finalNewBaseName.trim() === '') {
+      throw new errors.ErrorPolykeyCLIRenameSecret(
+        `EEMPTYNAME: The new secret name derived from '${newSecretNameArg}' is empty.`,
+      );
+    }
+    if (finalNewBaseName.includes('/') || finalNewBaseName.includes(':')) {
+      throw new errors.ErrorPolykeyCLIRenameSecret(
+        `EINVALIDCHAR: The final new secret name '${finalNewBaseName}' (derived from '${newSecretNameArg}') is invalid. It cannot contain '/' or ':' characters.`,
+      );
+    }
+    return finalNewBaseName;
+  }
 }
 
 export default CommandRename;

tests/secrets/rename.test.ts

@@ -33,6 +33,7 @@ describe('commandRenameSecret', () => {
       logger: logger,
     });
   });
+
   afterEach(async () => {
     await polykeyAgent.stop();
     await fs.promises.rm(dataDir, {
@@ -41,42 +42,106 @@ describe('commandRenameSecret', () => {
     });
   });
 
-  test('should rename secrets', async () => {
-    const vaultName = 'vault' as VaultName;
+  test('should rename secrets using a simple new name', async () => {
+    const vaultName = 'vaultSimpleRename' as VaultName;
     const vaultId = await polykeyAgent.vaultManager.createVault(vaultName);
-    const secretName = 'secret';
-    const newSecretName = 'secret-renamed';
-    const secretContent = 'this is the secret';
+    const oldSecretName = 'secretOriginal';
+    const newSecretBaseName = 'secretNewBase'; // Changed variable name for clarity
+    const secretContent = 'this is the secret for simple rename';
     await polykeyAgent.vaultManager.withVaults([vaultId], async (vault) => {
-      await vaultOps.addSecret(vault, secretName, secretContent);
+      await vaultOps.addSecret(vault, oldSecretName, secretContent);
     });
     command = [
       'secrets',
       'rename',
       '-np',
       dataDir,
-      `${vaultName}:${secretName}`,
-      newSecretName,
+      `${vaultName}:${oldSecretName}`,
+      newSecretBaseName, // Use the simple base name
     ];
     const result = await testUtils.pkStdio(command, {
       env: { PK_PASSWORD: password },
       cwd: dataDir,
     });
     expect(result.exitCode).toBe(0);
+    expect(result.stderr).toBe(''); // Expect no errors
     await polykeyAgent.vaultManager.withVaults([vaultId], async (vault) => {
       const list = await vaultOps.listSecrets(vault);
-      expect(list.sort()).toStrictEqual([newSecretName]);
+      expect(list.sort()).toStrictEqual([newSecretBaseName]);
+      expect(list).not.toContain(oldSecretName);
+    });
+  });
+
+  // New test case for the fix
+  test('should rename secret when new name is a fully qualified path', async () => {
+    const vaultName = 'vaultFQRename' as VaultName; // Use a distinct vault name for the test
+    const vaultId = await polykeyAgent.vaultManager.createVault(vaultName);
+    const oldSecretName = 'oldSecretName';
+    const newSecretBaseName = 'newSecretNameByPath'; // The actual target base name
+    const secretContent = 'content for fully qualified path rename test';
+
+    // Add initial secret
+    await polykeyAgent.vaultManager.withVaults([vaultId], async (vault) => {
+      await vaultOps.addSecret(vault, oldSecretName, secretContent);
+    });
+
+    // Construct the fully qualified path for the new secret name
+    // This format matches the problematic case: VaultName:/NewName
+    const newSecretFullyQualified = `${vaultName}:/${newSecretBaseName}`;
+
+    command = [
+      'secrets',
+      'rename',
+      '-np',
+      dataDir,
+      `${vaultName}:${oldSecretName}`, // Source secret path
+      newSecretFullyQualified, // New secret name as fully qualified path
+    ];
+
+    const result = await testUtils.pkStdio(command, {
+      env: { PK_PASSWORD: password },
+      cwd: dataDir,
+    });
+
+    expect(result.exitCode).toBe(0);
+    expect(result.stderr).toBe(''); // Expect no errors
+
+    // Verify the rename
+    await polykeyAgent.vaultManager.withVaults([vaultId], async (vault) => {
+      const secretsList = await vaultOps.listSecrets(vault);
+      expect(secretsList).toContain(newSecretBaseName); // Check for the base name
+      expect(secretsList).not.toContain(oldSecretName);
+      expect(secretsList.length).toBe(1); // Assuming only this secret is in the test vault
     });
   });
+
   test('should not rename vault root', async () => {
-    const vaultName = 'vault' as VaultName;
+    const vaultName = 'vaultRootTest' as VaultName; // Use a distinct vault name
     await polykeyAgent.vaultManager.createVault(vaultName);
-    command = ['secrets', 'rename', '-np', dataDir, vaultName, 'rename'];
-    const result = await testUtils.pkStdio(command, {
+    // Attempting to rename the vault itself, or an empty path within it
+    command = [
+      'secrets',
+      'rename',
+      '-np',
+      dataDir,
+      `${vaultName}:/`,
+      'newNameForRoot',
+    ];
+    let result = await testUtils.pkStdio(command, {
       env: { PK_PASSWORD: password },
       cwd: dataDir,
     });
     expect(result.exitCode).not.toBe(0);
     expect(result.stderr).toInclude('EPERM');
+
+    // Original test for trying to rename the vault name directly (which parseSecretPath handles as [vaultName, undefined, undefined])
+    // The `CommandRename` checks secretPath[1] == null, which covers `vaultName` (no colon) for the first arg.
+    command = ['secrets', 'rename', '-np', dataDir, vaultName, 'rename'];
+    result = await testUtils.pkStdio(command, {
+      env: { PK_PASSWORD: password },
+      cwd: dataDir,
+    });
+    expect(result.exitCode).not.toBe(0);
+    expect(result.stderr).toInclude('EPERM'); // This is because secretPath[1] will be undefined
   });
 });