Fix: Derive Gerrit credentials from live inputs

[ModeSevenIndustrialSolutions]

Important

Stacked on #393. This branch descends from fix/open-issues, so its two commits appear here too. Review only the last commit, Fix: Derive Gerrit credentials from live inputs. They will drop out once #393 merges.

Closes #394

The bug

derive_gerrit_credentials was @lru_cache(maxsize=32) keyed on (gerrit_host, organization, gerrit_port), while its answer depended on three things absent from that key:

Read	Source	In key?
_get_respect_user_ssh_setting()	G2G_RESPECT_USER_SSH env var	no
get_ssh_user_for_gerrit(...)	~/.ssh/config on disk	no
_get_cached_git_user_email()	git config user.email	no

That flag selects between two entirely different credential sources — the user's own SSH identity, or the {organization}.gh2gerrit service account. So a stale entry does not return a slightly wrong answer; it returns a developer's personal SSH username and email where the service account was required.

Before:

respect=true  -> ('modesevenindustrialsolutions', 'mwatkins@linuxfoundation.org')
respect=false -> ('modesevenindustrialsolutions', 'mwatkins@linuxfoundation.org')
identical: True

After, with no cache clearing between the two calls:

respect=true  -> ('modesevenindustrialsolutions', 'mwatkins@linuxfoundation.org')
respect=false -> ('myorg.gh2gerrit', 'releng+myorg-gh2gerrit@linuxfoundation.org')
identical: False

This was latent, not live. cli._load_effective_inputs sets the flag at line 2330 and the only chain reaching the cache starts at line 2344, so the first read happened after the mutation — verified by instrumenting the call. It sat one new caller or one reordering away from being live, with no error to point at a cache.

The two helpers were lru_cache(maxsize=1) over zero-argument functions, which is a global variable with a confusing spelling rather than a cache: with no key there is one slot, so any second caller collides with the first.

What changed

Nothing in this module is cached any more. The three memos are gone, and so is the SSH-config cache that the first revision of this PR tried to make sound.

_get_cached_git_user_email is now _read_git_user_email, since the old name no longer described it. Every docstring claiming caching was rewritten rather than left to mislead.

Why the SSH-config cache went too

The first revision kept it, keyed on (path, st_dev, st_ino, st_mtime_ns, st_size). Review found two holes, both real:

• Not thread-safe. The snapshot-then-evict-then-insert sequence lets two callers evict the same key, and the second raises KeyError. Bulk mode runs a ThreadPoolExecutor, and the module docstring advertises thread safety.
• A stat tuple is not a content fingerprint. An in-place rewrite can preserve inode and size, and coarse-timestamp filesystems or mtime-preserving tools can preserve st_mtime_ns. Adding st_dev/st_ino covered atomic write-and-rename — the easy case — but not the one that matters.

A correct key means hashing the file, which is most of the cost of parsing it, so the cache would pay nearly its own price to avoid itself. That prompted the question I should have asked first — what is it saving?

SSHConfig.load(): median 0.182 ms  max 0.493 ms   (50-host, 1629-byte config)

Once or twice per process, and zero times under GitHub Actions, where respect_user_ssh is false. So it went, and the reasoning is recorded where it used to sit so nobody reinvents it. The module now holds no process-wide state at all, which makes the thread-safety claim in its docstring true by construction rather than by locking correctly.

Removed API

clear_ssh_config_cache() and clear_credential_cache() are deleted, along with the ten setup_method calls to them and the autouse fixture. (An earlier revision of this description promised to keep them; that is no longer accurate.)

These are internal helpers of an application, not library API — __init__.py exports only __version__, there is no __all__, and neither name appears in any documentation. Their only callers were this repository's tests. Keeping them as no-ops would be worse than removing them: a function that answers "cleared" while guaranteeing nothing reproduces, in miniature, exactly the invisible-state problem this PR exists to fix. There is also nothing left to want them for, since every input is now read afresh on each call.

Cost of removing the memos

Measured rather than assumed, since avoiding repeated work was their stated purpose:

	after cli.py:2344	after cli.py:767
derive_gerrit_credentials	1	2
get_git_user_email	1	2
SSHConfig.load()	1	2

get_git_user_email() costs ~12 ms (two subprocesses: a git --version validation plus the lookup); SSHConfig.load() ~0.2 ms. In GitHub Actions mode these sources are read zero times.

Net: one extra git config lookup and one extra config parse, ~12 ms, once per process, local mode only. Against _load_effective_inputs at 236 ms and a run that clones over the network, the memos were buying noise at the price of a wrong-identity failure mode.

Validation

Gate	Result
pytest	1550 passed, 0 failed, 0 errors, 0 skipped
basedpyright (124 files)	0 errors, 0 warnings
ruff check / format	clean, no suppressions added anywhere
aislop ci	100 / 100, 0 findings
prek run --all-files	every hook passed

Suite went 1549 → 1553 with the new tests, then → 1550 as three tests were deleted with their subject; delta fully accounted for.

Non-vacuity proven by restoring the decorators — all four new tests fail, the first with exactly the reported symptom:

E AssertionError: assert ('sshuser', 'dev@example.org') == ('testorg.gh2...undation.org')
FAILED ...::test_respect_user_ssh_flag_change_switches_source
FAILED ...::test_git_email_change_changes_derived_email
FAILED ...::test_ssh_config_rewrite_changes_derived_user
FAILED ...::test_ssh_config_appearing_later_is_picked_up

Worth a reviewer's eye

Three existing tests asserted the caching directly and could not survive its removal. Repurposed rather than deleted, each inverted to prove the opposite property — please sanity-check this is the right trade:

• test_derive_gerrit_credentials_caching → ..._rereads_its_sources; call_count == 1 → == 2
• test_respect_user_ssh_setting_caching → ..._reads_env_every_call; call_count == 1 → == 3

Three were deleted outright because their subject no longer exists: test_clear_credential_cache_functionality and the SSH-config polluter/victim pair in test_cache_isolation.py. The pair's property is covered more strongly by test_ssh_config_rewrite_changes_derived_user, which rewrites the same path in place — the pair used two different tmp_path directories, which a path-keyed cache would have separated anyway — with get_ssh_user_for_gerrit left unmocked, so only a genuine re-read yields the new answer.

Also noted, unchanged: derive_gerrit_credentials now runs twice per local run rather than once, from each of its two call sites. Same inputs, same answer — only the work repeats. And TestCaching in tests/unit/test_ssh_config_parser.py is now a misnomer; its docstring is corrected but the class name is left alone to avoid churning node IDs.

[Copilot]

Pull request overview

Removes unsound credential memoization so Gerrit credentials reflect live environment, Git identity, and SSH configuration.

Changes:

• Re-reads credential sources on every derivation.
• Invalidates parsed SSH configuration using file metadata.
• Adds regression tests for changing credential inputs.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/github2gerrit/ssh_config_parser.py	Updates credential and SSH-config caching.
tests/unit/test_ssh_config_parser.py	Adds live-input regression tests.
tests/test_cache_isolation.py	Verifies credential state does not leak.
tests/conftest.py	Stacked #393 test isolation change.
src/github2gerrit/cli.py	Stacked #393 provenance change.
src/github2gerrit/config.py	Stacked #393 provenance change.
src/github2gerrit/duplicate_detection.py	Stacked #393 duplicate-resolution change.
tests/test_config_helpers.py	Stacked #393 tests.
tests/test_duplicate_detection.py	Stacked #393 tests.
tests/test_gerrit_urls_more.py	Stacked #393 cache-test change.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.