[claude-tmgbedu]

Fixes task #3504.

Root cause

env() returns a dataclasses.Field with a default_factory; it only becomes a value when the class is processed by @dataclass. A Config subclass without @dataclass was never processed, so DatabaseConfig().default fell through to the class attribute — the raw Field.

Fix

_ConfigMeta.__call__ now lazily applies dataclass at first instantiation to every unprocessed Config class in the MRO (base first, so undecorated parents' fields are inherited). Decorated classes are untouched (eager __init_subclass__ processing was rejected: re-decoration turns every env() field into a required argument). Casting follows existing env() rules (int/bool by default type).

ADR 035 added and indexed.

Tests

• tests/support/test_config_without_dataclass.py: unset/set variable, int/bool casting, kwargs override, undecorated parent+child, decorated class not reprocessed (5/6 fail on main).
• Full suite (--ignore=tests/masoniteorm/postgres): 3294 passed, coverage 91.4%.

🤖 Generated with Claude Code

https://claude.ai/code/session_01SchgSwsSKsBpqmyLiFEuM1

[codecov]

Codecov Report

❌ Patch coverage is 97.67442% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...pi_startkit/src/fastapi_startkit/support/config.py	97.67%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[claude-tmgbedu]

Verdict: REQUEST CHANGES. GitHub won't let me request changes on a PR from the same account, so this is posted as a comment.

Suite passes locally (3294 passed, Postgres ignored). The direct repro is fixed and the decorated-only path is unchanged. One inheritance case regresses, though.

1. support/config.py ensure_dataclass: a decorated child of an undecorated parent loses the parent's fields (regression).

   class P(Config):
       host: str = env("E_HOST", "localhost")
   @dataclass
   class C(P):
       port: int = env("E_PORT", 80)
   C().host   # AttributeError; fields(C) == ['port']

   @dataclass on C runs before P has been processed, so C never inherits host. On the first C() call, ensure_dataclass processes P, which deletes the Field class attribute. On main this returned the raw Field; now it raises AttributeError. The ADR's claim that undecorated parents are "processed first" only holds when every class in the chain is undecorated.
   Fix: process undecorated Config ancestors when a subclass is defined, for example in _ConfigMeta.__init__, or in Config.__init_subclass__, over cls.__mro__[1:]. The parent's body is complete by then and the child's decorator hasn't run yet. Only the leaf would stay lazy. Add a test for this case.

2. The Config base itself is mutated on first instantiation. reversed(cls.__mro__) includes Config, so the first X() turns Config into a dataclass (__eq__ is added and __hash__ becomes None). That is global, order-dependent state. Either skip Config (klass is not Config) or make Config an explicit @dataclass up front.

3. Thread safety (non-blocking, but say how it's handled). The check-then-decorate is not atomic. I couldn't trigger a race in 300 tries with 8 threads, but two threads constructing a never-instantiated class at the same moment would both run dataclass(klass). Either guard it with a threading.Lock (double-checked), or record in the ADR why concurrent double-processing is harmless. Also, dataclass(Q); dataclass(Q); Q() worked for me on CPython 3.15, which contradicts the reason the ADR gives for rejecting alternative 2. Please re-check that claim, or state which Python versions it applies to.

The ADR, its index entry and the no-comments convention all look fine otherwise.

[claude-tmgbedu]

Re-review (Code Reviewer 3017) — REQUEST CHANGES

Full suite without Postgres: 3299 passed, 10 skipped, coverage gate OK (Python 3.15.0). I checked each prior point with a script against PR head 28736968 and against main.

Prior points

1. @dataclass child of undecorated parent — ✅ fixed. fields(C) == ['host','port']. A 3-level chain (undecorated → undecorated → @dataclass) also inherits all fields.
2. Config base turned into a dataclass / hash — ✅ fixed. is_dataclass(Config) is False, Config.__hash__ is object.__hash__, and undecorated instances stay hashable (eq=False).
3. Lock on lazy decoration — ✅ fixed. The check runs again inside _dataclass_lock, and the 8-thread barrier test confirms a single decoration.
4. ADR rationale — ✅ ADR 035 now gives the Py3.15 double-processing reason (frozen env() values, and ValueError on default_factory=dict), and the index entry was added.

New regressions (blocking)

ensure_dataclass now runs dataclass(eq=False) on every undecorated Config subclass, including ones that never use env()/field() and that work fine on main. With the PR, these now fail at first instantiation:

1. fastapi_startkit/src/fastapi_startkit/support/config.py:21-31: an undecorated child of a frozen dataclass parent

   @dataclass(frozen=True)
   class FP(Config): x: str = env("X", "x")
   class FC(FP): pass
   FC()   # main: works -> 'x'   PR: TypeError: cannot inherit non-frozen dataclass from a frozen one

   (With the ancestor processing in __init_subclass__, a grandchild of FC fails at class definition time.)
2. Same location: a mutable class default

   class Mut(Config): opts: dict = {}
   Mut()  # main: {}   PR: ValueError: mutable default <class 'dict'> ... use default_factory

3. Same location: an annotation without a default

   class Req(Config): name: str
   Req()  # main: works   PR: TypeError: missing 1 required positional argument: 'name'

Suggested fix: in is_undecorated (or a separate predicate), auto-process a class only when its own __dict__ holds at least one dataclasses.Field value, i.e. the class actually relies on env()/field(). Classes without a Field keep exactly the main behaviour, and every case this PR fixes still works. Intermediate classes that are skipped are fine: dataclass reads inherited __dataclass_fields__ through getattr. Please add regression tests for the three cases above, and add a line to ADR 035.

Non-blocking

• An undecorated subclass with its own __init__ that doesn't set an env() field now gets AttributeError/missing attribute on that field, because dataclass deletes the class attribute. On main it returned the raw Field, which was broken anyway. Optionally note it in the ADR.

[claude-tmgbedu]

Round-2 re-review (Code Reviewer 3017) — APPROVE

Head 41fca3bb. Full suite without Postgres: 3305 passed, 10 skipped, coverage gate OK (Python 3.15.0). I re-ran last round's script against this head.

Points from the last round

• (a) An undecorated child of a frozen dataclass parent works as on main (FC().x == 'x'). ✅
• (b) opts: dict = {} without env() works as on main ({}). ✅
• (c) A bare name: str without env() works as on main. ✅

Earlier fixes still hold

The repro prints 'sqlite'. A @dataclass child of an undecorated parent, and a 3-level chain, inherit all fields. Config is not a dataclass and keeps object.__hash__. The re-check under the lock is unchanged, and the concurrency test passes. Dropping klass is not Config is safe because Config.__dict__ holds no Field.

Frozen propagation and the documented edge

• An env() child of a frozen parent is processed with frozen=True. I checked frozen → plain → env → env grandchild: all fields resolve. ✅ has_frozen_base reads __dataclass_params__ from the nearest dataclass ancestor, which is correct.
• A class body that mixes env() fields with opts: dict = {} raises ValueError, as ADR 035 says. That class only returned raw Fields on main, so the edge is acceptable. ✅
• needs_dataclass only looks at the class's own __dict__, and the mixed-hierarchy test confirms that a plain middle class is skipped while the leaf still collects the base's fields.

Non-blocking (ADR follow-up)

• A @dataclass(frozen=True) child of an undecorated env() parent now raises TypeError: cannot inherit frozen dataclass from a non-frozen one at class definition. __init_subclass__ processes the parent as non-frozen before the child's decorator runs. On main the class defined fine, but the parent's fields were raw Fields, so it never worked fully. The child's decorator arguments can't be seen from __init_subclass__, so this is unavoidable. Please add one line to ADR 035 next to the mixed-body edge, saying the parent should be decorated @dataclass(frozen=True) in that case.
• (Still true from last round) An undecorated class with its own __init__ that doesn't set its env() fields has no attribute for them.

Not merging.

[claude-tmgbedu]

Round-3 re-review (Code Reviewer 3017) — REQUEST CHANGES

Head 7faa62a1. Full suite without Postgres: 3310 passed, 10 skipped, coverage gate OK (Python 3.15.0). The earlier cases still behave: the repro, inheritance chains, frozen handling, Config base, the lock, bare annotations and the frozen child without annotations. The MemoryDiskConfig motivation works, and plain-default classes now accept kwargs. Constants, methods, properties, forward-ref annotations and eq/hash (identity) all behave.

The widened rule does cause four regressions vs main for undecorated classes that use plain defaults and no env(). Each one was checked with the same script on both branches.

1. support/config.py copy_unhashable_defaults: ClassVar with an unhashable default now raises.

   class C(Config):
       registry: ClassVar[dict] = {}
       driver: str = "memory"
   C()  # main: OK   PR: TypeError: field registry cannot have a default factory

   Fix: skip ClassVar (and InitVar) annotations, using the same typing.ClassVar check dataclasses uses, or leave such defaults alone.

2. A class-level mutable default is no longer shared, and mutations are silently lost. After the first instantiation, dataclass deletes the class attribute, and _ConfigMeta.__getattr__ returns a fresh deepcopy on every access:

   class Shared(Config): registry: dict = {}
   a = Shared(); Shared.registry["k"] = 1
   Shared.registry, Shared().registry, a.registry
   # main: ({'k':1}, {'k':1}, {'k':1})   PR: ({}, {}, {})

   Whether the mutation sticks also depends on whether it happened before or after the first instantiation. This is silent data loss with no error.

3. A custom __init__ combined with an unhashable default now raises AttributeError.

   class C(Config):
       opts: dict = {}
       def __init__(self): pass
   C().opts  # main: {}   PR: AttributeError: 'C' object has no attribute 'opts'

4. Unhashable non-container defaults are deepcopied at every instantiation. A class that defines __eq__ without __hash__ (common for clients and value objects) counts as unhashable:

   class WithObj(Config): client: Holder = Holder()   # Holder holds a threading.Lock
   WithObj()  # main: shared instance   PR: TypeError: cannot pickle '_thread.lock' object

   Even when deepcopy succeeds, the per-instance copy changes identity semantics and costs one deepcopy per field on every instantiation.

Suggested fix (minimal)

• Apply the plain-defaults branch of needs_dataclass only when every own default is hashable and the class does not define __init__ in its own __dict__. That covers MemoryDiskConfig (str/int/bool defaults) and leaves registries, clients and custom-init classes exactly as on main.
• Drop copy_unhashable_defaults, or limit it to classes that already hold an env()/field() Field (the round-2 mixed-body case), restricted to exact dict/list/set defaults.
• In both paths, skip ClassVar/InitVar annotations.
• Add regression tests for cases 1–4, and update ADR 035, which currently says unhashable plain defaults are "copied per instance" without these caveats.

Not merging.

[claude-tmgbedu]

Round-4 re-review (Code Reviewer 3017) — APPROVE

Head 8cfe8195. Full suite without Postgres: 3322 passed, 10 skipped, coverage gate OK (Python 3.15.0). I re-ran the round 1–3 scripts against this head and compared with main.

Round-3 points, each now identical to main

1. registry: ClassVar[dict] = {} next to a plain field gives ('memory', {}), no error. ✅
2. A shared class-level registry: dict = {} mutated after the first instantiation gives ({'k':1}, {'k':1}, {'k':1}). ✅
3. A custom __init__ with opts: dict = {} gives ('n', {}). ✅
4. An eq-only object holding a Lock is shared as before, and no deepcopy remains in the code. ✅

Earlier rounds still hold

The repro prints 'sqlite'. The @dataclass-child, 3-level and mixed chains inherit their fields. Config is not a dataclass and keeps object.__hash__. The lock and frozen propagation are unchanged, and the frozen child without annotations and bare annotations behave as on main. Constants, methods, properties, forward-ref strings and positional/keyword args behave (kwargs are now accepted where main raised TypeError).

New logic checked

• has_custom_init: the first __init__ found in the MRO decides. object and dataclass-generated __init__s don't count, and a mixin __init__ makes the class skip processing, as on main. The pending list is computed before parents are processed, but parents carry no __init__ of their own at that point, so the result doesn't change. ✅
• String ClassVar/InitVar resolution against the module namespace: typing.ClassVar/aliased-module forms resolve, and an unresolvable string counts as a field, which is the safe direction. ✅
• Shallow .copy is limited to exact dict/list/set in Field-bearing classes, and ADR 035 describes it accurately.

Non-blocking: please add one line to ADR 035

• Class-level overrides after the first instantiation are ignored for processed plain-default classes.

  class C(Config): driver: str = "memory"
  C(); C.driver = "patched"
  C().driver   # main: 'patched'   PR: 'memory'   (an override set before the first instantiation still applies)

  This is standard @dataclass semantics: every decorated config in the repo already behaves this way, and nothing in the repo overrides config classes like this (I grepped). It does affect monkeypatch.setattr(SomeConfig, ...) in user tests, so it deserves a sentence telling users to pass kwargs or set the env var instead.
• Similarly, in an env() + opts: dict = {} class, Cls.opts returns a fresh copy on each access. That class only returned raw Fields on main, so this is acceptable as documented.

Not merging.