Fix 13 latent bugs found by a full-source audit

Metaclass (parser.py):
- Copy a shared Argument() instance before inferring type/nargs/choices:
  one instance bound to several fields/classes was mutated in place, so
  `b: str = arg` silently required the int type inferred for `a: int`.
- Reject arguments that would shadow methods/properties (parse_args,
  print_help, user-defined helpers): the metaclass used to replace them
  with `...` placeholders, silently destroying the parser API.
- Reject a bare `serve: Serve` annotation (no instance): it silently
  became a required CLI argument with type=<Parser class>.
- Inherit unannotated arguments, groups, and subparsers: previously a
  subclass lost every `tags = Argument(...)`, `db = DB()` or
  `serve = Serve()` declared on its base class.
- Preserve inherited plain defaults: `name: str = "x"` used to become
  a *required* argument in every subclass because the processed class
  attribute had been replaced with an Ellipsis placeholder.
- Wrap each element of Secret(nargs=...) lists in SecretString; only
  scalar secrets were masked, so repr(list) leaked the values.
- Remove dead code in the Optional-unwrap branch.

Config parsers (defaults.py, actions.py):
- Make strict=False consistent: malformed INI/TOML files are now
  skipped like JSON ones instead of raising; strict=True still raises.
- Alias INIDefaultsParser.BOOL_TRUE_VALUES to types.TEXT_TRUE_VALUES
  (the two identical sets could drift apart).
- ConfigAction caches results via a None sentinel instead of
  truthiness, so an empty config is parsed once.

Factory (factory.py):
- EnumArgument(lowercase=True) now uses an explicit name map instead
  of str.upper() round-trips, fixing enums with non-uppercase member
  names; enum aliases (e.g. LogLevelEnum.WARN) remain accepted.

Docs (pitfalls.md):
- Unselected subcommand attributes raise AttributeError; the page
  claimed they "retain their default values".
- Document that Group/subparser attributes are class-level singletons
  shared by all instances of a Parser class.

tests/test_audit_fixes.py covers every fix; 21 of its 38 tests fail
against the pre-fix code.

Author: mosquito

argclass/actions.py

@@ -70,7 +70,10 @@ def __call__(
         values: str | Any | None,
         option_string: str | None = None,
     ) -> None:
-        if not self._result:
+        # ``None`` is the not-yet-parsed sentinel; an empty dict is a
+        # valid cached result (truthiness would re-parse it and, on a
+        # second invocation, try Path() on the mapping in values).
+        if self._result is None:
             filenames: Sequence[Path] = list(self.search_paths)
             if values:
                 filenames = [Path(values)] + list(filenames)

argclass/defaults.py

@@ -11,6 +11,7 @@
 from collections.abc import Iterable, Mapping
 
 from .exceptions import ConfigurationError
+from .types import TEXT_TRUE_VALUES
 from .utils import own_section_items
 
 try:
@@ -166,29 +167,31 @@ class INIDefaultsParser(AbstractDefaultsParser):
     when requested via get_value() with appropriate ValueKind.
     """
 
-    # Values considered as True for boolean conversion
-    BOOL_TRUE_VALUES = frozenset(
-        (
-            "true",
-            "yes",
-            "1",
-            "on",
-            "enable",
-            "enabled",
-            "t",
-            "y",
-        )
-    )
+    # Values considered as True for boolean conversion. Aliased to the
+    # shared constant so this set and ``argclass.parse_bool`` cannot
+    # drift apart; kept as a class attribute so subclasses can still
+    # override it.
+    BOOL_TRUE_VALUES = TEXT_TRUE_VALUES
 
     def parse(self) -> Mapping[str, Any]:
         parser = configparser.ConfigParser(
             allow_no_value=True,
             strict=self._strict,
         )
 
-        filenames = self._filter_readable_paths()
-        loaded = parser.read(filenames)
-        self._loaded_files = tuple(Path(f) for f in loaded)
+        loaded: list[Path] = []
+        for path in self._filter_readable_paths():
+            # Mirror the JSON/TOML parsers: in non-strict mode a
+            # malformed file is skipped (best effort), in strict mode
+            # the error propagates.
+            try:
+                read_ok = parser.read([path])
+            except (configparser.Error, OSError):
+                if self._strict:
+                    raise
+                continue
+            loaded.extend(Path(f) for f in read_ok)
+        self._loaded_files = tuple(loaded)
 
         result: dict[str, Any] = dict(
             parser.items(parser.default_section, raw=True),
@@ -275,7 +278,10 @@ def parse(self) -> Mapping[str, Any]:
                     if isinstance(data, dict):
                         result.update(data)
                         loaded_files.append(path)
-            except OSError:
+            # TOMLDecodeError subclasses ValueError in both stdlib
+            # tomllib and the tomli fallback, so a malformed file is
+            # skipped in non-strict mode just like JSON/INI.
+            except (OSError, ValueError):
                 if self._strict:
                     raise
 

argclass/factory.py

@@ -537,27 +537,36 @@ def EnumArgument(
     Returns:
         TypedArgument instance.
     """
+    # Map accepted spellings to members explicitly instead of relying
+    # on ``str.upper()`` round-trips, which break for enums whose
+    # member names are not fully uppercase (``Fast`` -> ``FAST`` ->
+    # KeyError). ``__members__`` includes aliases (e.g. ``WARN`` for
+    # ``WARNING``) so they keep working as inputs; ``choices`` lists
+    # canonical names only.
+    members = enum_class.__members__
     if lowercase:
+        name_map = {n.lower(): m for n, m in members.items()}
         choices = tuple(e.name.lower() for e in enum_class)
     else:
+        name_map = dict(members)
         choices = tuple(e.name for e in enum_class)
 
     if default is not None:
         if isinstance(default, enum_class):
             pass  # Valid enum member
         elif isinstance(default, str):
             # Validate string is a valid enum member name
-            check_name = default.upper() if lowercase else default
-            valid_names = tuple(e.name for e in enum_class)
-            if check_name not in valid_names:
+            member = members.get(default)
+            if member is None and lowercase:
+                member = name_map.get(default.lower())
+            if member is None:
                 raise EnumValueError(
                     f"default {default!r} is not a valid {enum_class.__name__} "
                     f"member",
                     enum_class=enum_class,
-                    valid_values=valid_names,
+                    valid_values=tuple(e.name for e in enum_class),
                 )
-            # Convert string default to enum member
-            default = enum_class[check_name]
+            default = member
         else:
             raise EnumValueError(
                 f"default must be {enum_class.__name__} member or string, "
@@ -574,9 +583,12 @@ def converter(x: Any) -> Any:
         # Handle existing enum members
         if isinstance(x, enum_class):
             return x.value if use_value else x
-        # Convert string to enum
-        name = x.upper() if lowercase else x
-        member = enum_class[name]
+        # Convert string to enum member via the explicit name map
+        member = name_map.get(x.lower() if lowercase else x)
+        if member is None:
+            # Same failure mode as enum_class[x]; parse_args wraps
+            # converter errors into TypeConversionError.
+            raise KeyError(x)
         return member.value if use_value else member
 
     return TypedArgument(

argclass/parser.py

@@ -91,6 +91,59 @@ def reserved_name_error(name: str) -> ArgumentDefinitionError:
     )
 
 
+def shadowed_member_error(name: str, member: Any) -> ArgumentDefinitionError:
+    kind = "property" if isinstance(member, property) else "method"
+    return ArgumentDefinitionError(
+        f"Field {name!r} would shadow the {kind} {name!r} defined on a "
+        f"base class or in the class body; argclass would silently "
+        f"replace it and break the parser API.",
+        field_name=name,
+        hint="Rename the field, or pass the callable explicitly via "
+        "Argument(default=...) if a callable default is intended.",
+    )
+
+
+def _shadowed_api_member(
+    key: str,
+    attrs: Mapping[str, Any],
+    bases: tuple[type, ...],
+) -> Any | None:
+    """Return the method/property that a member named ``key`` would
+    clobber, or ``None`` when the name is safe to use.
+
+    Registering ``key`` as an argument replaces the class attribute
+    (the metaclass stores ``...`` placeholders and ``parse_args``
+    assigns parsed values), so a name that currently resolves to a
+    callable or property — ``parse_args``, a user-defined helper —
+    must be rejected instead of silently destroying the API.
+    Inherited argclass members (arguments, groups, subparsers and
+    their ``...`` placeholders) and plain data defaults remain
+    legitimate redefinition targets.
+    """
+    own = attrs.get(key)
+    if (
+        own is not None
+        and not isinstance(own, (TypedArgument, AbstractGroup, AbstractParser))
+        and (
+            callable(own)
+            or isinstance(own, (property, classmethod, staticmethod))
+        )
+    ):
+        return own
+    for base in bases:
+        if not hasattr(base, key):
+            continue
+        value = getattr(base, key)
+        if value is None or value is Ellipsis:
+            return None
+        if isinstance(value, (TypedArgument, AbstractGroup, AbstractParser)):
+            return None
+        if callable(value) or isinstance(value, property):
+            return value
+        return None
+    return None
+
+
 class Meta(ABCMeta):
     """Metaclass for Parser and Group classes."""
 
@@ -113,9 +166,31 @@ def __new__(
         # Annotations defined directly on this class (not inherited).
         own_annotations = own_annotation_keys(cls)
 
-        arguments = {}
-        argument_groups = {}
-        subparsers = {}
+        # Seed the registries with inherited members so unannotated
+        # arguments, groups, and subparsers declared on a base class
+        # survive subclassing (annotated fields are re-collected below
+        # from the merged annotations either way).
+        arguments: dict[str, Any] = {}
+        argument_groups: dict[str, Any] = {}
+        subparsers: dict[str, Any] = {}
+        for base in reversed(bases):
+            arguments.update(getattr(base, "__arguments__", {}))
+            argument_groups.update(getattr(base, "__argument_groups__", {}))
+            subparsers.update(getattr(base, "__subparsers__", {}))
+
+        # Keys (re)defined by this class itself, as opposed to seeded.
+        own_keys: set[str] = set()
+
+        def classify(key: str, value: Any, into: dict[str, Any]) -> None:
+            # A redefinition may change category (e.g. an inherited
+            # argument overridden by a group); drop the stale entry
+            # from the other registries.
+            for registry in (arguments, argument_groups, subparsers):
+                if registry is not into:
+                    registry.pop(key, None)
+            into[key] = value
+            own_keys.add(key)
+
         for key, kind in annotations.items():
             if key in RESERVED_ATTRIBUTES:
                 # Inherited internal attributes are skipped; declaring one
@@ -126,13 +201,63 @@ def __new__(
             if key.startswith("_"):
                 continue
 
+            # A purely inherited member (not re-annotated and not
+            # re-assigned here) is already fully processed in the
+            # seeded registries. Rebuilding it from the annotation
+            # would lose state the base class attrs no longer carry:
+            # plain defaults become ``...`` placeholders on the class,
+            # so a re-build would turn ``name: str = "x"`` into a
+            # required argument in every subclass.
+            if (
+                key not in own_annotations
+                and key not in attrs
+                and (
+                    key in arguments
+                    or key in argument_groups
+                    or key in subparsers
+                )
+            ):
+                continue
+
             try:
                 argument = deep_getattr(key, attrs, *bases)
                 has_explicit_default = True
             except KeyError:
                 argument = None
                 has_explicit_default = False
 
+            shadowed = _shadowed_api_member(key, attrs, bases)
+            if shadowed is not None:
+                raise shadowed_member_error(key, shadowed)
+
+            # Subparsers are defined by instances, not annotations: a
+            # bare ``serve: Serve`` (or ``Serve | None``) would
+            # otherwise fall through and silently become a required
+            # CLI argument with ``type=Serve``.
+            parser_kind: type | None = None
+            if isinstance(kind, type) and issubclass(kind, AbstractParser):
+                parser_kind = kind
+            else:
+                try:
+                    _inner = unwrap_optional(kind)
+                except ComplexTypeError:
+                    _inner = None
+                if isinstance(_inner, type) and issubclass(
+                    _inner, AbstractParser
+                ):
+                    parser_kind = _inner
+            if parser_kind is not None and not isinstance(
+                argument, AbstractParser
+            ):
+                raise ArgumentDefinitionError(
+                    f"Subparser field '{key}' is annotated with parser "
+                    f"class {parser_kind.__name__} but no instance is "
+                    f"assigned; an annotation alone cannot define a "
+                    f"subcommand.",
+                    field_name=key,
+                    hint=f"Use '{key} = {parser_kind.__name__}()'.",
+                )
+
             annotated_group_cls: type[AbstractGroup] | None = None
             if isinstance(kind, type) and issubclass(kind, AbstractGroup):
                 annotated_group_cls = kind
@@ -292,12 +417,15 @@ def __new__(
 
             if isinstance(argument, TypedArgument):
                 if argument.type is None and argument.converter is None:
+                    # The same Argument() instance may be shared by
+                    # several fields or classes; mutate a private copy
+                    # so this field's inferred type/nargs/choices do
+                    # not leak into the other bindings.
+                    argument = argument.copy()
                     # First try to unwrap optional
                     optional_inner = unwrap_optional(kind)
                     if optional_inner is not None:
                         kind = optional_inner
-                        if argument.default is None:
-                            argument.default = None
 
                     # Handle bool type: set STORE_TRUE/STORE_FALSE action
                     if kind is bool and argument.action == Actions.default():
@@ -339,9 +467,14 @@ def __new__(
                             argument.converter = container_type
                     else:
                         argument.type = kind
-                arguments[key] = argument
+                classify(key, argument, arguments)
             elif isinstance(argument, AbstractGroup):
-                argument_groups[key] = argument
+                classify(key, argument, argument_groups)
+            else:
+                # Only Parser instances can reach this point: the
+                # block above converts every non-argclass value into
+                # a TypedArgument.
+                classify(key, argument, subparsers)
 
         for key, value in attrs.items():
             if key in RESERVED_ATTRIBUTES:
@@ -357,15 +490,22 @@ def __new__(
                 continue
 
             # Skip if already processed from annotations
-            if key in arguments or key in argument_groups or key in subparsers:
+            if key in own_keys:
                 continue
 
+            if isinstance(
+                value, (TypedArgument, AbstractGroup, AbstractParser)
+            ):
+                shadowed = _shadowed_api_member(key, attrs, bases)
+                if shadowed is not None:
+                    raise shadowed_member_error(key, shadowed)
+
             if isinstance(value, TypedArgument):
-                arguments[key] = value
+                classify(key, value, arguments)
             elif isinstance(value, AbstractGroup):
-                argument_groups[key] = value
+                classify(key, value, argument_groups)
             elif isinstance(value, AbstractParser):
-                subparsers[key] = value
+                classify(key, value, subparsers)
 
         setattr(cls, "__arguments__", MappingProxyType(arguments))
         setattr(cls, "__argument_groups__", MappingProxyType(argument_groups))
@@ -909,8 +1049,16 @@ def parse_args(
                     parsed_value = getattr(parsed_ns, key)
 
                 if argument is not None:
-                    if argument.secret and isinstance(parsed_value, str):
-                        parsed_value = SecretString(parsed_value)
+                    if argument.secret:
+                        if isinstance(parsed_value, str):
+                            parsed_value = SecretString(parsed_value)
+                        elif isinstance(parsed_value, (list, tuple)):
+                            # nargs secrets: wrap each element so a
+                            # repr of the list cannot leak the values.
+                            parsed_value = type(parsed_value)(
+                                SecretString(v) if isinstance(v, str) else v
+                                for v in parsed_value
+                            )
                     if argument.converter is not None:
                         if argument.nargs and parsed_value is None:
                             parsed_value = []