fix(auto-install): bind STAMP_PATH/OPT_OUT_PATH defaults at call time (#839)

[robotrocketscience]

Closes #839.

Bug

maybe_install_manifest in src/aelfrice/auto_install.py bound stamp_path and opt_out_path to the module-level STAMP_PATH / OPT_OUT_PATH constants at function-definition time:

def maybe_install_manifest(
    *,
    installed_version: str,
    scope: SettingsScope = "user",
    settings_path: Path | None = None,
    stamp_path: Path = STAMP_PATH,        # bound at def time
    opt_out_path: Path = OPT_OUT_PATH,    # bound at def time
    ...
)

Tests that monkeypatch.setattr(auto_install, "STAMP_PATH", tmp_path/...) and then call auto_install_at_cli_entry(...) were silently ignored — the merge ran against the user's real ~/.aelfrice/installed-manifest-version and ~/.aelfrice/opt-out-hooks.json. I tripped this myself when writing tests for #834 / PR #836.

Fix

Two-line signature change + four-line body addition. Defaults are None; resolution happens inside the function from the module globals (mirrors the existing settings_path pattern at the top of the same function).

def maybe_install_manifest(
    *,
    installed_version: str,
    scope: SettingsScope = "user",
    settings_path: Path | None = None,
    stamp_path: Path | None = None,
    opt_out_path: Path | None = None,
    force: bool = False,
    timeout: int | None = None,
) -> AutoInstallResult:
    ...
    if stamp_path is None:
        stamp_path = STAMP_PATH
    if opt_out_path is None:
        opt_out_path = OPT_OUT_PATH
    ...

Existing call sites that pass paths explicitly are unaffected. The cli.py call path (auto_install_at_cli_entry → maybe_install_manifest(installed_version=...)) takes the None-default branch and resolves to the real STAMP_PATH / OPT_OUT_PATH, byte-identical behaviour for production.

Test

test_module_attr_monkeypatch_propagates_to_default_args — monkeypatches auto_install.STAMP_PATH to a tmp file, calls maybe_install_manifest(installed_version="2.2.0", settings_path=...) with no explicit stamp_path, asserts the tmp stamp file got the new version. If the bound-default regression ever comes back the assertion fails (because the real ~/.aelfrice/installed-manifest-version would have been written instead).

The pre-existing test_auto_install_at_cli_entry_runs_when_uv_tool docstring was tightened — it called out the bound-default leak as the rationale for stubbing maybe_install_manifest. With this PR the leak is fixed; the stub now stands on test-isolation grounds alone.

Full suite: 4276 passed, 62 skipped, 75 xfailed.

Out of scope

• Same audit for read_stamp / write_stamp / read_opt_outs / add_opt_out / remove_opt_out. They all default to STAMP_PATH / OPT_OUT_PATH at function-def time too. They are not the load-bearing merge path that ends up writing user state during a CLI invocation, and refactoring them risks broader API churn for negligible gain. Worth filing as a separate cleanup if a test-side reason ever surfaces.
• Changing cli.py:6472 to pass paths through explicitly. The None-default contract is the cleaner shape; the call site stays exactly as it is today.

Tier

rook — single function, two-line signature change + four-line body addition + one test + one CHANGELOG entry. No production behaviour change.

Summary by CodeRabbit

• Bug Fixes

  • Ensure configuration path defaults are resolved at call time instead of at definition time, fixing test-time overrides and improving reliability of path-based behavior.

• Tests

  • Added a regression test that verifies runtime overriding of module-level path values is respected by default parameters.

[sourcery-ai]

Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 13f0e745-5036-4ae9-8dda-d9ea9f359de3

📥 Commits

Reviewing files that changed from the base of the PR and between c74c109 and b67aec6.

📒 Files selected for processing (3)

• CHANGELOG/v3.md
• src/aelfrice/auto_install.py
• tests/test_auto_install.py

✅ Files skipped from review due to trivial changes (1)

• CHANGELOG/v3.md

🚧 Files skipped from review as they are similar to previous changes (2)

• tests/test_auto_install.py
• src/aelfrice/auto_install.py

---

📝 Walkthrough

Walkthrough

This PR changes maybe_install_manifest so stamp_path and opt_out_path default to None and are resolved to STAMP_PATH / OPT_OUT_PATH at call time instead of binding at function-definition time. It adds a regression test that monkeypatches the module attributes and a changelog entry documenting the fix.

Changes

Deferred default resolution for install paths

Layer / File(s)	Summary
Function signature + runtime defaults src/aelfrice/auto_install.py	maybe_install_manifest signature: `stamp_path: Path
Regression test that validates monkeypatching tests/test_auto_install.py	New test test_module_attr_monkeypatch_propagates_to_default_args monkeypatches auto_install.STAMP_PATH/OPT_OUT_PATH, invokes maybe_install_manifest without passing those args, and asserts the stamp is written to the monkeypatched path. Also shortens an unrelated test docstring.
Changelog entry CHANGELOG/v3.md	Adds a Fixed bullet documenting that stamp_path and opt_out_path defaults now resolve at call time and referencing the new regression test.

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: fixing the binding of STAMP_PATH/OPT_OUT_PATH defaults to occur at call time instead of definition time, with issue reference.
Description check	✅ Passed	The description comprehensively covers the bug, fix, test, and scope. It includes all template sections: summary (bug and fix), linked issues (Closes #839), type of change (fix), verification checklist, test plan, and notes.
Linked Issues check	✅ Passed	The PR fully implements the requirements from issue #839: changes stamp_path/opt_out_path defaults to None, resolves them at call-time from module globals, adds test_module_attr_monkeypatch_propagates_to_default_args regression test, and keeps changes minimal.
Out of Scope Changes check	✅ Passed	All changes are tightly scoped to the fix described in issue #839. Changelog entry, function signature, docstring, and new test are all directly related to the call-time binding objective; out-of-scope items are explicitly deferred.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/issue-839-stamp-path-call-time-defaults

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[robotrocketscience]

[claim:review:bagheera:2026-05-15T03:18:25Z]

[robotrocketscience]

Review

Substance LGTM. Branch is out of date relative to current github/main — needs rebase before merge (see Blocker below).

What's right

• Fix mirrors the existing settings_path pattern in the same function (auto_install.py:351). settings_path: Path | None = None resolved inside the body via target_path = settings_path if settings_path is not None else USER_SETTINGS_PATH. PR #841 extends the same pattern to stamp_path / opt_out_path with in-place rebinding (if stamp_path is None: stamp_path = STAMP_PATH). Either shape is correct; the in-place rebind is actually slightly cleaner because the resolved value is used at 4 separate sites downstream (read_stamp, stamp_path.parent.mkdir, lock_path = stamp_path.parent / ..., _do_merge(stamp_path=...)).

• Byte-identical production behaviour. Verified the lone production call site at auto_install.py:552 (auto_install_at_cli_entry → maybe_install_manifest(installed_version=installed_version)) — both stamp_path and opt_out_path are omitted there. Post-fix that path takes the None-default branch and resolves to module-level STAMP_PATH / OPT_OUT_PATH, identical to the pre-fix bound default.

• Regression test correctly exercises the property under test. test_module_attr_monkeypatch_propagates_to_default_args monkeypatches auto_install.STAMP_PATH to a tmp path, calls maybe_install_manifest(installed_version=..., settings_path=...) with no explicit stamp_path, then asserts read_stamp(stamp) == "2.2.0" against the tmp file. If a future refactor restores the bound-default, the assertion fails because the merge would have written to the real ~/.aelfrice/installed-manifest-version. The test docstring also explicitly names the failure mode, which keeps the why-it-exists trail intact.

• Out-of-scope notes are right. Same-shape audit for read_stamp / write_stamp / read_opt_outs / add_opt_out / remove_opt_out is correctly deferred — they all have the same def-time-bound pattern but aren't on the load-bearing merge path that writes user state.

Blocker — needs rebase

The CHANGELOG diff against current github/main is wrong:

-## [3.2.0] - 2026-05-15
...
-[Unreleased]: ...compare/v3.2.0...HEAD
-[3.2.0]: ...compare/v3.1.0...v3.2.0
+[Unreleased]: ...compare/v3.0.1...HEAD

The PR removes the [3.2.0] - 2026-05-15 section heading and the corresponding compare-link footnote, plus the [Unreleased] compare-link reverts from v3.2.0...HEAD to v3.0.1...HEAD. That's because PR #841 was opened against a base that predates the v3.2.0 release commit 840a7924 (merged 2026-05-15T03:14:21Z, ~1h ago).

Rebase onto current github/main. After rebase the CHANGELOG entry for #839 lands under [Unreleased] and the [3.2.0] section + footnote stay put. Code/test changes should be conflict-free.

Minor / non-blocking

• The CHANGELOG entry is single-paragraph and tells the why-clearly. Could go a little shorter ("monkeypatching auto_install.STAMP_PATH is silently ignored because the default arg captures the module attribute at def time; fix uses None defaults + body resolution"), but the current length matches the surrounding entry style on v3.md and the in-band rationale is useful — not worth churning over.

Verdict

Approve after rebase. CI is fully green on the current head; no concerns there. The substantive change is the right shape and the regression test is direct.

[robotrocketscience]

[release:review:bagheera:2026-05-15T03:21:12Z]

[robotrocketscience]

[claim:review:prince:2026-05-15T03:23:05Z]

[robotrocketscience]

Reviewed 77eb8f8a → f4f2f67b → b67aec6a (rebased from c74c109a onto current main 840a7924).

Rebase resolution (CHANGELOG/v3.md): the v3.2.0 release (#835, 840a7924) drained [Unreleased] into a dated [3.2.0] section between when this PR was opened and when the merge-train fired. The original docs(changelog) commit added the #839 Fixed entry as the first child of [Unreleased]/### Fixed, which conflicted with the #833 entry that landed in the same slot under [3.2.0]. Resolved by:

• Keeping the #833 entry in [3.2.0]/### Fixed (HEAD side — unchanged).
• Moving the #839 entry into the now-empty [Unreleased]/### Fixed (with scaffolding ### Fixed). This is the post-release shape — new fixes go into [Unreleased] until the next release migrates them.

Per-commit content unchanged from the pre-rebase versions; the resolution is mechanical and content-preserving.

Fix is correct. Two-arg signature change + four-line body addition. Defaults move from Path = STAMP_PATH / Path = OPT_OUT_PATH (bound at function-def time) to Path | None = None with if x is None: x = STAMP_PATH resolution inside the body. Mirrors the existing settings_path pattern at the top of the same function. Caller-side: existing call sites passing paths explicitly are unaffected; module-attribute monkeypatches now propagate.

This closes the exact testability foot-gun I called out as out-of-scope in my #836 review — test_auto_install_at_cli_entry_runs_when_uv_tool had to stub maybe_install_manifest end-to-end rather than exercise it because of this issue. The new test_module_attr_monkeypatch_propagates_to_default_args regression pins the property in place going forward.

Verification:

• 3 atomic signed commits (%G? = G each): fix, test, changelog.
• Diff vs main: +53/-7 across src/aelfrice/auto_install.py, tests/test_auto_install.py, CHANGELOG/v3.md.
• Discretion grep on main...HEAD — clean.
• release-docs-check will exit 0 (no pyproject.toml version bump). The new [Unreleased]-drain assertion from #838 passes — [Unreleased] now has one entry but the assertion only fires on release PRs.

Out of scope (per PR body, agree): auditing read_stamp / write_stamp / read_opt_outs / add_opt_out / remove_opt_out for the same def-time-binding pattern. They share the constants but aren't on the user-state-writing path, so the same bug class is less load-bearing there. Worth a follow-up if someone wants to lock the pattern repo-wide.

Re-adding ready-to-merge, dropping attn:unblock.

[robotrocketscience]

[release:review:prince:2026-05-15T03:25:03Z]

[github-actions]

merge-train: merged b67aec6 → main via FF push.