teng-lin--notebooklm-py/docs/adr/0007-test-monkeypatch-policy.md

# ADR-0007: Test-monkeypatch policy — constructor-injection via `tests/_fixtures/`

## Status

Accepted.

This ADR shipped in the `arch-d1-fixtures-scaffolding` PR (D1 PR-1). It defines the *forbidden* test patterns going forward. The migration of the existing offenders to constructor seams is **complete**: the meta-lint's file-level allowlist has been **drained to zero** (issue #1376), so the per-file gate is now a global invariant and `tests/_guardrails/test_no_forbidden_monkeypatches.py` asserts `_ALLOWLIST == frozenset()` as a hardening guard against re-growth. The policy and lint stay in force indefinitely; only the *migration* is finished.

**Shim-retirement milestone complete (issue #1367).** All three named production shims this ADR set out to remove are now gone: `_AuthFacadeModule` (retired by ADR-0014), the `_core.py` property-bridge zoo (retired in the session-shrink arc per ADR-0001), and — as of #1367 ([PR #1374](https://github.com/teng-lin/notebooklm-py/pull/1374)) — the last one, the `cli/session_cmd` patch-surface bridge (`_resolve_paths_helper` precedence chain plus its dual fixture and the pure re-export surface). The earlier "only the `cli/session_cmd` block remains / two of three retired" status is superseded by this entry. With the shims retired *and* the allowlist drained to zero (issue #1376), both halves of the policy — shim retirement and offender migration — are complete, so this ADR is now plain "Accepted".

**Coverage update (issue #1325):** the original meta-lint policed only the three `monkeypatch.setattr` / direct-`AsyncMock`-assignment forms below and was blind to the `unittest.mock` channel — `mock.patch("notebooklm._private…")` / `patch("notebooklm._private…")` / `patch.object(notebooklm._private…, ...)` — which is where most of the recent growth happened. As of #1325 the lint also flags those string-target patches into *private* `notebooklm._*` paths (forbidden pattern 4 below), and the file-level allowlist gained the pre-existing offenders so the policy now covers both mocking channels. Those offenders have since migrated to constructor seams along with the rest, leaving the allowlist empty (issue #1376).

**Coverage update (2026-06, test-seam layout-coupling gap):** pattern 4's regex anchors on `notebooklm\._` — the private component must be the *first* path component — so the lint was structurally blind to **deep-leaf** private string targets like `patch("notebooklm.cli.session_cmd._sync_server_language_to_config")`, the exact form the #1481 CLI command-move post-mortem showed silently no-op'ing when a command body relocates. The lint now also flags (5) deep-leaf private string targets (at least one public component before the first `_`-prefixed one — deliberately **disjoint** from pattern 4, which keeps its drained-to-zero allowlist and stays-empty guard) and (6) private-*attribute-name* `patch.object(alias, "_private_attr")` patches, where the object reference is sound but the attribute string pins internal layout (only *full* dunder names — leading and trailing double underscore, e.g. `"__aenter__"` — are exempt; `"__private"`-style names are still flagged). Each new pattern carries its own **baselined** file-level allowlist (`_DEEP_LEAF_ALLOWLIST`, `_PATCH_OBJECT_PRIVATE_ATTR_ALLOWLIST`) holding the offenders measured at gate-landing time; unlike the original allowlist these start populated and drain opportunistically (stale-entry checks force removal of cleaned/deleted files, and no new entries are accepted). In the same change, the *remaining* layout coupling — string-target `patch("notebooklm…")` at **public** leaves, which no forbidden-pattern rule covers — was frozen by a per-file shrink-only population ratchet, `tests/_guardrails/test_string_patch_ratchet.py`: baselined files are pinned at their measured site counts, unbaselined files have a budget of zero, and ceilings only tighten. The two gates cross-reference each other in their remediation text so a violator is steered to the sanctioned seams (constructor injection via `tests/_fixtures/`, or `patch.object` on a *public* attribute of a locally-imported alias) rather than into the other gate's forbidden shape.

## Context

An internal architecture audit identifies "test-monkeypatch gravity" as the first of three architectural diseases — a *gravity well* in which the test suite's preferred mocking strategy welds module boundaries shut and forces every seam extraction to ship a write-through facade, a property bridge, or a parallel-implementation invariant.

The audit's verified counts at HEAD `22355cf` are:

| Pattern | Count |
|---|---|
| Total `monkeypatch.setattr` sites under `tests/` | 236 |
| String-target patches — `monkeypatch.setattr("notebooklm.X.Y", …)` | 58 |
| Object-attribute patches — `monkeypatch.setattr(obj, "attr", …)` | 152 |
| Direct attribute assignment — `target.rpc_call = AsyncMock(…)` etc. | 63 |
| `tests/unit/test_auth_*.py` split (concern-aligned: `test_auth_storage.py`, `test_auth_account.py`, `test_auth_refresh.py` etc.) | Formerly 4,090 LOC · 70 patches |
| `tests/unit/cli/test_session.py` size | 4,431 LOC |

Each of these patterns shares one root cause: production code is mutated from the outside *after* construction, rather than receiving its collaborators *during* construction. At the time this ADR was written, three artifacts visible in `src/notebooklm/` existed solely to keep that style working as the architecture shifts beneath it. **All three are now retired** (see the Status block); they are described here in their original form for historical context:

- `_AuthFacadeModule` (`auth.py:288-339`) — a `types.ModuleType` subclass whose `__setattr__` mirrors writes from `notebooklm.auth.<name>` across `_auth/storage`, `_auth/account`, `_auth/keepalive`, `_auth/refresh`, plus header/cookie/policy seams. The shim existed because ~152 patches target the `notebooklm.auth` namespace and would silently no-op if the facade were a passive re-export. **Retired** — see ADR-0003 (closed by ADR-0014).
- The `_core.py` property-bridge zoo (lines 450-774, ~324 LOC) — read/write properties that delegate to the seam where the storage actually lives. They existed because `monkeypatch.setattr(core, "_save_lock", fake)` was a load-bearing idiom across dozens of tests. **Retired** in the session-shrink arc — see ADR-0001 and ADR-0002.
- The `cli/session_cmd.py` proxy block (lines 141-490, ~350 LOC) — module-level functions that mirror service-layer symbols so `monkeypatch.setattr("notebooklm.cli.session_cmd.X", fake)` reaches the real implementation in `cli/services/login.py`. **Retired** by #1367 ([PR #1374](https://github.com/teng-lin/notebooklm-py/pull/1374)), which removed the `_resolve_paths_helper` precedence chain, its dual fixture, and the pure re-export surface — the last of the three named shims to go.

Every seam extraction up through tier 10 had to add or extend one of these structures rather than break the patches. The cost compounds: each new refactor that crossed one of these load-bearing boundaries either preserved the shim (cementing it) or broke ~100 tests at once. With all three now retired, that gravity is gone for the named shims, and the follow-on work — draining the meta-lint's file-level allowlist as the older offending tests migrate to constructor injection — is **complete: the allowlist drained to zero** (issue #1376).

The mitigation is a policy change, not a code change. New tests stop using these patterns; existing ones migrate to constructor injection through factories. With no new offenders entering the codebase, the shims become finite, retire-able artifacts rather than permanent fixtures.

## Decision

Going forward, every test that needs to substitute a collaborator on `Session` or any sub-client (`NotebooksAPI`, `SourcesAPI`, `ArtifactsAPI`, `ChatAPI`, `ResearchAPI`, `NotesAPI`, `SettingsAPI`, `SharingAPI`) **must** acquire that collaborator through constructor injection, using the factory substrate in `tests/_fixtures/`:

```python
from tests._fixtures import make_fake_core

async def test_notebooks_list_returns_payload() -> None:
    fake = make_fake_core(rpc_call=AsyncMock(return_value=[fake_payload]))
    api = NotebooksAPI(fake.rpc_executor)
    result = await api.list()
    fake.rpc_executor.rpc_call.assert_awaited_once()
```

The following patterns are **forbidden** in new test code and enforced by `tests/_guardrails/test_no_forbidden_monkeypatches.py`:

1. **String-target monkeypatches into the `notebooklm` namespace** — `monkeypatch.setattr("notebooklm.X.Y", ...)`. These rely on import-string resolution and silently no-op when storage relocates.
2. **Object-attribute monkeypatches via the `notebooklm` module** — `monkeypatch.setattr(notebooklm.X, "attr", ...)`. Same failure mode; written slightly differently.
3. **Direct AsyncMock attribute assignment to the transport/RPC surface** — `target.rpc_call = AsyncMock(...)`, `target._perform_authed_post = AsyncMock(...)`, `target._begin_transport_post = AsyncMock(...)`, `target._finish_transport_post = AsyncMock(...)`, `target.query_post = AsyncMock(...)`, including chained variants like `self._client._target.rpc_call = AsyncMock(...)`. These mutate a constructed instance instead of injecting a fake at construction.
4. **`unittest.mock` string-target patches into private internals** (added in #1325) — `mock.patch("notebooklm._private…")` / `patch("notebooklm._private…")` / `patch.object(notebooklm._private…, ...)`. Same import-string failure mode as (1), routed through `unittest.mock` instead of `monkeypatch`. Scoped to paths whose *first* component is private (`notebooklm._*`); deep-leaf privates are pattern (5), and patches at public facades are out of scope for the forbidden-pattern rules (their population is frozen by the string-patch ratchet instead — see the 2026-06 coverage update in the Status block).
5. **Deep-leaf `unittest.mock` string-target patches into private internals** (added with the 2026-06 coverage update) — `patch("notebooklm.<public…>._private…")`, e.g. `patch("notebooklm.cli.session_cmd._sync_server_language_to_config")`. The same failure mode as (4) with the private component behind one or more public ones; disjoint from (4) by construction and carries its own baselined, opportunistically-draining file-level allowlist (`_DEEP_LEAF_ALLOWLIST`).
6. **Private-attribute-name `patch.object` via a local alias** (added with the 2026-06 coverage update) — `patch.object(alias, "_private_attr", …)`. The object reference is real, but the attribute *name* string pins internal attribute layout and keeps "passing" against bag-of-attribute fakes after a rename. Only full dunder names (`"__aenter__"` etc.) are exempt — Python-protocol surface, not internal layout. Baselined in `_PATCH_OBJECT_PRIVATE_ATTR_ALLOWLIST`; `patch.object` on a *public* attribute remains the sanctioned seam-alias form.

`tests/_fixtures/fake_core.py` provides `make_fake_core(**overrides) -> FakeSession`. `FakeSession` is a plain explicit-attribute bag whose defaults cover the current shared runtime Protocols in `src/notebooklm/_runtime/contracts.py` (`RpcCaller`, `LoopGuard`, `Kernel`) plus the single-consumer seams that tests still exercise, such as upload auth metadata, artifact polling scope, drain-hook registration, source-list lookup, and queue-wait recording. The factory exposes `fake.rpc_executor.rpc_call` to mirror production wiring and keeps `fake.rpc_call` as a legacy convenience that points at the same mock. Defaults are benign `AsyncMock`s for the async surface and `MagicMock`s for the sync surface; tests override only the slice they exercise via keyword arguments.

The choice of `types.SimpleNamespace`-shaped attribute storage (rather than `MagicMock(spec=...)` against the fat union or a Protocol-typed class) keeps the factory's per-attribute assignment explicit, prevents `MagicMock` from auto-vivifying attributes the production code does not actually use, and lets reviewers diff the factory against the narrow Protocols a sub-client structurally requires.

`tests/_fixtures/conftest.py` exposes exactly two thin pytest fixtures — `fake_core` (default factory call) and `make_fake_core` (the factory itself, for tests that need per-call overrides). The deliberately small fixture surface avoids the failure mode `/grill-me` Q11 flagged: a "full fixture menu" creates one parameterless pytest fixture per per-test override combination, which scales O(N tests × M overrides) and quickly becomes worse than the monkeypatch pattern it replaces.

The meta-lint (`tests/_guardrails/test_no_forbidden_monkeypatches.py`) runs in `pytest` (no skip markers), scans each test file as a single string (so multi-line `monkeypatch.setattr(\n  "notebooklm.X", …)` forms are caught — `\s` already matches newlines in Python's `re` engine), and reports each violation with `(file, line, matched pattern)` for actionable failure messages. Its file-level allowlist started at 49 files (the union of audit-flagged offenders at PR-start) and was driven toward zero as offenders migrated. **The allowlist drained to zero** (issue #1376): issue #1325 added a second batch of pre-existing offenders when the lint's coverage was extended to the `unittest.mock` channel (forbidden pattern 4) — files that patch private `notebooklm._*` paths via `mock.patch` / `patch` string targets, concentrated in `notebooklm._research`, `notebooklm._artifact.downloads`, and `notebooklm._sources` — and those migrated to constructor seams along with the rest. The stale-entry guard (a file with no remaining offenders fails the lint) drove the list down without rot, and now that it is empty the lint additionally asserts `_ALLOWLIST == frozenset()` so it cannot re-grow: a new offender must be migrated, never re-allowlisted.

## Consequences

**Wanted:**

- New tests cannot rebuild the gravity well. The meta-lint blocks the patterns at PR review.
- Sub-clients become testable in isolation. `NotebooksAPI(core=fake)` exercises the sub-client without standing up a real `Session`, without import-string resolution, and without after-construction mutation.
- The `_AuthFacadeModule` shim, the `_core.py` property bridges, and the `cli/session_cmd.py` proxy block were always intended as *removable* artifacts. They kept existing tests passing until those tests migrated; **all three have now been deleted with no remaining test churn** (D2 cutover and D1 PR-2/PR-3 removed the first two; #1367 / [PR #1374](https://github.com/teng-lin/notebooklm-py/pull/1374) removed the `cli/session_cmd` bridge). The file-level allowlist drain is **complete — the allowlist drained to zero** (issue #1376).
- Test diffs become smaller and more readable. A test that overrides one collaborator now shows one keyword argument instead of one `monkeypatch.setattr` line per substituted symbol.

**Unwanted:**

- Tests that previously relied on string-target patches to substitute imports from modules they don't directly construct (e.g. patching `notebooklm._core.asyncio.sleep` to skip a real delay) need a different mechanism. The migration plan absorbs these on a case-by-case basis. Most either move to constructor injection of a clock-like collaborator or switch from the *string-target* form — `monkeypatch.setattr("notebooklm._auth.refresh.X", …)`, which the lint forbids because string-target patches resolve at import time and silently no-op on relocation — to the *object-attribute* form against a locally-imported seam alias, which the lint accepts because the alias variable provides a real Python object reference rather than a string the lint cannot validate. The seam-aliased form is the recommended migration target for legitimate `unittest.mock.patch`-style needs that don't fit the constructor-injection model.
- The factory carries a small amount of duplication with the narrow Protocols defined in `src/notebooklm/_runtime/contracts.py` and the remaining single-consumer seams in feature modules. Keeping the factory aligned with those Protocols is a manual step; the alternative — generating it — would couple test plumbing to source code in ways that defeat the "tests don't pin shape" property the policy is trying to recover.
- The initial allowlist is non-trivial (49 files). Migrating them is a large project, scheduled explicitly as D1 PR-2 (auth-side, 6 files) and D1 PR-3 (CLI-side, 7 files), with the residual ~35 files reassessed at PR-3 close.

## Alternatives considered

- **Pure pytest fixtures (no factory function).** Rejected per `/grill-me` Q11. A fixture-only world requires one `@pytest.fixture` per override combination — `fake_core_with_rpc_returning_x`, `fake_core_with_authuser_42`, `fake_core_raising_chat_error_on_post`, and so on. The combinatorial explosion is worse than the current monkeypatch sprawl. Hybrid factory-first (factory takes `**overrides`, plus two thin fixtures wrapping the no-override and the factory-itself cases) keeps per-test customization at the call site where it's most readable.
- **`MagicMock(spec=SessionCapabilities)` instead of an explicit dataclass.** Rejected. `MagicMock(spec=…)` auto-vivifies attributes against the spec, which means: (a) it silently accepts attribute access the sub-client doesn't actually use, defeating the type-safety benefit of the narrow Protocols, and (b) it ties the test factory to the fat-union `SessionCapabilities` *which D2 PR-2 deletes*. A `types.SimpleNamespace`-shaped explicit class survives the D2 cutover and surfaces shape drift as a hard `AttributeError` rather than as a silent auto-vivified `MagicMock`.
- **Per-site allowlist entries (one allowlist entry per `(file, line)` pair).** Rejected per `/grill-me` Q12. Line numbers change on every rebase, generating spurious merge conflicts in the allowlist and making the meta-lint's failures look like merge churn rather than real findings. File-level allowlists trade a small amount of precision (we accept "this file still has offenders") for a stable substrate that survives reorderings.
- **One big-bang test rewrite (delete the 273 sites in a single PR).** Rejected. The audit measured 4,090 LOC in `test_auth.py` and 4,431 LOC in `cli/test_session.py`; touching both atomically would block every other test-touching PR for the duration of the cutover and offer no incremental verification. The chosen migration sequence (D1 PR-1 ships the substrate; D1 PR-2 migrates auth tests and deletes `_AuthFacadeModule`; D1 PR-3 migrates CLI tests and deletes the proxy block + property bridges) lets each PR's pytest run prove that the *next* shim removal is safe.
- **Deferring the lint to a CI-only check (mypy plugin or separate runner).** Rejected. A pytest-resident lint runs on every contributor's local `uv run pytest` and surfaces violations before push, which is where new violations are easiest to fix. A CI-only check would let local development continue to introduce offenders; the round-trip cost (push → CI fail → fix → push) is exactly the friction the policy is trying to remove.
- **Why this matters now.** The audit's 273-site gravity-well analysis (`arch-biggest-problem-audit.md` §D1) shows the cost compounds with every refactor: each new seam extraction either ships a new shim or breaks ~100 tests. Deferring the policy means the next architectural arc (tier 12+ or a hypothetical greenfield split) pays the same tax at higher magnitude. Establishing the substrate now — when the shims that need retirement are still finite and named — keeps the eventual migration bounded.