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) — 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), 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/:

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 AsyncMocks for the async surface and MagicMocks 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 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.