Fix 13 code review issues: bugs, security, compatibility, and cleanup

- Fix duplicate save/load/commit/whoami method definitions in fluent API
- Fix from_json() mutating input dict via .pop() (now uses .get())
- Fix Node.is_file/is_dir/is_device/is_symlink to use proper POSIX bitmask
- Replace os.system('clear') with ANSI escape sequence
- Replace bare except: clauses with specific exception types
- Fix Python 3.8 compat: tuple[str,str] → Tuple[str,str]
- Sync __init__.py version to 0.2.1, fix placeholder author/email
- Fix ls -l to show actual permissions via _format_mode()
- Remove dead duplicate head/tail handler in CommandExecutor
- Remove double history recording in run_interactive()
- Move inline imports to module-level, remove unused imports
- Add 23-test regression suite for all fixed issues

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: queelius

CITATION.cff

@@ -0,0 +1,11 @@
+cff-version: 1.2.0
+message: "If you use this software, please cite it as below."
+title: "dagshell"
+version: "0.2.1"
+license: MIT
+repository-code: "https://github.com/queelius/dagshell"
+url: "https://pypi.org/project/dagshell/"
+authors:
+  - family-names: "Towell"
+    given-names: "Alexander"
+    orcid: "https://orcid.org/0000-0001-6443-9897"

dagshell/__init__.py

@@ -6,9 +6,9 @@
 interface and an embedded Scheme interpreter.
 """
 
-__version__ = "0.1.0"
-__author__ = "Your Name"
-__email__ = "your.email@example.com"
+__version__ = "0.2.1"
+__author__ = "Alex Towell"
+__email__ = "lex@metafunctor.com"
 
 from .dagshell import (
     FileSystem,

dagshell/dagshell.py

@@ -8,16 +8,16 @@
 - Simple, composable operations following Unix philosophy
 """
 
+import base64
+import builtins
 import hashlib
 import json
-import time
 import os
+import time
+from collections import defaultdict
 from dataclasses import dataclass, field, asdict
-from typing import Dict, List, Optional, Set, Union, Any
 from enum import IntEnum
-from collections import defaultdict
-import random
-import string
+from typing import Dict, List, Optional, Set, Tuple, Union, Any
 
 
 class Mode(IntEnum):
@@ -65,19 +65,19 @@ def to_dict(self) -> dict:
 
     def is_file(self) -> bool:
         """Check if this is a regular file."""
-        return (self.mode & Mode.IFDIR) == 0 and (self.mode & Mode.IFCHR) == 0
+        return (self.mode & 0o170000) == Mode.IFREG
 
     def is_dir(self) -> bool:
         """Check if this is a directory."""
-        return (self.mode & Mode.IFDIR) != 0
+        return (self.mode & 0o170000) == Mode.IFDIR
 
     def is_device(self) -> bool:
         """Check if this is a device."""
-        return (self.mode & Mode.IFCHR) != 0
+        return (self.mode & 0o170000) == Mode.IFCHR
 
     def is_symlink(self) -> bool:
         """Check if this is a symbolic link."""
-        return (self.mode & Mode.IFLNK) == Mode.IFLNK
+        return (self.mode & 0o170000) == Mode.IFLNK
 
 
 @dataclass(frozen=True)
@@ -98,7 +98,6 @@ def __init__(self, content: Union[str, bytes] = b"", mode: int = Mode.FILE_DEFAU
     def to_dict(self) -> dict:
         d = super().to_dict()
         # Store content as base64 for JSON serialization
-        import base64
         d['content'] = base64.b64encode(self.content).decode('ascii')
         d['type'] = 'file'
         return d
@@ -328,7 +327,7 @@ def _resolve_path(self, path: str, follow_symlinks: bool = True,
 
         return node_hash
 
-    def _get_parent_path(self, path: str) -> tuple[str, str]:
+    def _get_parent_path(self, path: str) -> Tuple[str, str]:
         """Split path into parent directory and basename."""
         path = os.path.normpath(path)
         if path == '/':
@@ -706,7 +705,7 @@ def _create_file(self, path: str, content: bytes, mode: str, mtime: Optional[flo
 
     # User and permission management
 
-    def lookup_user(self, username: str) -> tuple[int, int]:
+    def lookup_user(self, username: str) -> Tuple[int, int]:
         """Look up user ID and primary group ID from /etc/passwd."""
         passwd_hash = self._resolve_path('/etc/passwd')
         if not passwd_hash:
@@ -813,8 +812,6 @@ def export_to_real(self, target_path: str, preserve_permissions: bool = True,
         Returns:
             Number of files/directories exported
         """
-        import shutil
-
         # Default mappings (virtual -> real)
         if uid_map is None:
             uid_map = {0: 0, 1000: os.getuid(), 1001: os.getuid(), 1002: os.getuid()}
@@ -851,7 +848,7 @@ def export_to_real(self, target_path: str, preserve_permissions: bool = True,
                     mode = node.mode & 0o777
                     try:
                         os.chmod(real_path, mode)
-                    except:
+                    except Exception:
                         pass  # Ignore permission errors
 
             elif node.is_file():
@@ -860,7 +857,6 @@ def export_to_real(self, target_path: str, preserve_permissions: bool = True,
                 os.makedirs(parent_dir, exist_ok=True)
 
                 # Write file content
-                import builtins
                 with builtins.open(real_path, 'wb') as f:
                     f.write(node.content)
                 exported += 1
@@ -870,7 +866,7 @@ def export_to_real(self, target_path: str, preserve_permissions: bool = True,
                     mode = node.mode & 0o777
                     try:
                         os.chmod(real_path, mode)
-                    except:
+                    except Exception:
                         pass  # Ignore permission errors
 
                 # Try to set ownership if running as root
@@ -879,7 +875,7 @@ def export_to_real(self, target_path: str, preserve_permissions: bool = True,
                     real_gid = gid_map.get(node.gid, node.gid)
                     try:
                         os.chown(real_path, real_uid, real_gid)
-                    except:
+                    except Exception:
                         pass  # Ignore if can't change ownership
 
         return exported
@@ -905,8 +901,6 @@ def import_from_real(self, source_path: str, target_path: str = '/',
             FileNotFoundError: If source_path doesn't exist
             ValueError: If target_path is invalid
         """
-        import builtins
-
         if uid is None:
             uid = 1000
         if gid is None:
@@ -1046,13 +1040,12 @@ def from_json(cls, json_str: str) -> 'FileSystem':
         fs.refs = defaultdict(int)
 
         # Reconstruct nodes
-        import base64
         for hash, node_data in data['nodes'].items():
-            node_type = node_data.pop('type', 'file')
+            node_type = node_data.get('type', 'file')
 
             if node_type == 'file':
-                content = base64.b64decode(node_data.pop('content', ''))
-                node = FileNode(content=content, **{k: v for k, v in node_data.items() if k != 'content'})
+                content = base64.b64decode(node_data.get('content', ''))
+                node = FileNode(content=content, **{k: v for k, v in node_data.items() if k not in ('type', 'content')})
             elif node_type == 'dir':
                 node = DirNode(**{k: v for k, v in node_data.items() if k != 'type'})
             elif node_type == 'device':

dagshell/dagshell_fluent.py

@@ -318,6 +318,9 @@ def whoami(self) -> CommandResult:
         Usage:
             whoami
 
+        Examples:
+            whoami                 # Show current username
+
         Returns:
             Current username.
         """
@@ -623,15 +626,15 @@ def ls(self, path: Optional[str] = None,
                 files = [f for f in files if not f.startswith('.')]
 
         if long:
-            # Detailed listing (simplified for now)
+            # Detailed listing with actual permissions
             detailed = []
             for f in files:
                 full_path = os.path.join(target_path, f)
                 fstat = self.fs.stat(full_path)
                 if fstat:
-                    type_char = 'd' if fstat['type'] == 'dir' else '-'
+                    mode_str = self._format_mode(fstat.get('mode', 0o100644))
                     size = fstat.get('size', 0)
-                    detailed.append(f"{type_char}rw-r--r--  1 user user {size:8} {f}")
+                    detailed.append(f"{mode_str}  1 user user {size:8} {f}")
             result = CommandResult(data=files, text='\n'.join(detailed))
         else:
             result = CommandResult(data=files, text='\n'.join(files))
@@ -1196,8 +1199,6 @@ def grep(self, pattern: str, *paths: str,
         Returns:
             Lines matching the pattern.
         """
-        import re
-
         # Prepare regex
         flags = re.IGNORECASE if ignore_case else 0
         try:
@@ -2033,7 +2034,6 @@ def save(self, filename: str = 'dagshell.json') -> CommandResult:
             Status message.
         """
         try:
-            import json
             # Get JSON representation
             json_data = self.fs.to_json()
 
@@ -2065,7 +2065,6 @@ def load(self, filename: str = 'dagshell.json') -> CommandResult:
             Status message.
         """
         try:
-            import json
             # Read from real file
             with open(filename, 'r') as f:
                 json_data = f.read()
@@ -2131,26 +2130,6 @@ def export(self, target_path: str, preserve_permissions: bool = True) -> Command
             error = f"Export failed: {e}"
             return CommandResult(data=error, text=error, exit_code=1)
 
-    def whoami(self) -> CommandResult:
-        """Print effective username.
-
-        Usage:
-            whoami
-
-        Options:
-            None
-
-        Examples:
-            whoami                 # Show current username
-
-        Returns:
-            Current username.
-        """
-        # For now, return a default user
-        # This will be overridden by terminal session
-        username = "user"
-        return CommandResult(data=username, text=username, exit_code=0)
-
     def su(self, username: str = 'root') -> CommandResult:
         """Switch user account.
 
@@ -2173,86 +2152,6 @@ def su(self, username: str = 'root') -> CommandResult:
         result = f"Switched to user: {username}"
         return CommandResult(data=username, text=result, exit_code=0)
 
-    def save(self, filename: str = 'dagshell.json') -> CommandResult:
-        """Save virtual filesystem to JSON file.
-
-        Usage:
-            save [FILENAME]
-
-        Options:
-            FILENAME               File to save to (default: dagshell.json)
-
-        Examples:
-            save                   # Save to dagshell.json
-            save backup.json       # Save to backup.json
-
-        Returns:
-            Status message.
-        """
-        # Get filesystem state as JSON
-        json_data = self.fs.to_json()
-
-        # Write to real filesystem
-        with open(filename, 'w') as f:
-            f.write(json_data)
-
-        result = f"Filesystem saved to {filename}"
-        return CommandResult(data=json_data, text=result, exit_code=0)
-
-    def load(self, filename: str = 'dagshell.json') -> CommandResult:
-        """Load virtual filesystem from JSON file.
-
-        Usage:
-            load [FILENAME]
-
-        Options:
-            FILENAME               File to load from (default: dagshell.json)
-
-        Examples:
-            load                   # Load from dagshell.json
-            load backup.json       # Load from backup.json
-
-        Returns:
-            Status message.
-        """
-        try:
-            # Read from real filesystem
-            with open(filename, 'r') as f:
-                json_data = f.read()
-
-            # Create new filesystem from JSON
-            self.fs = dagshell.FileSystem.from_json(json_data)
-
-            # Reset working directory
-            self._cwd = '/'
-
-            result = f"Filesystem loaded from {filename}"
-            return CommandResult(data=json_data, text=result, exit_code=0)
-        except FileNotFoundError:
-            result = f"load: {filename}: No such file"
-            return CommandResult(data=None, text=result, exit_code=1)
-        except Exception as e:
-            result = f"load: {filename}: Error: {e}"
-            return CommandResult(data=None, text=result, exit_code=1)
-
-    def commit(self, filename: str = 'dagshell.json') -> CommandResult:
-        """Alias for save - commit virtual filesystem to JSON file.
-
-        Usage:
-            commit [FILENAME]
-
-        Options:
-            FILENAME               File to save to (default: dagshell.json)
-
-        Examples:
-            commit                 # Save to dagshell.json
-            commit state.json      # Save to state.json
-
-        Returns:
-            Status message.
-        """
-        return self.save(filename)
-
     def import_file(self, real_path: str, virtual_path: Optional[str] = None,
                     safe_paths: Optional[List[str]] = None, recursive: bool = True) -> CommandResult:
         """Import a file from the real filesystem into the virtual filesystem.
@@ -2279,9 +2178,6 @@ def import_file(self, real_path: str, virtual_path: Optional[str] = None,
         Returns:
             Status message or error.
         """
-        import os
-        import pathlib
-
         # Default safe paths if not specified
         if safe_paths is None:
             safe_paths = [
@@ -2410,8 +2306,6 @@ def export_file(self, virtual_path: str, real_path: Optional[str] = None,
         Returns:
             Status message or error.
         """
-        import os
-
         # Default safe paths if not specified
         if safe_paths is None:
             safe_paths = [