fix(core): serialise record_installation, format the credential-store 500 (#1085)

[frankbria]

Closes #1085.

1. record_installation did an unlocked read-modify-write

#954 made the write atomic, so a crash cannot truncate the file. That is a
different guarantee from serialised: two concurrent installs read the same
base, both write a whole new file, and the second one silently drops the first
tool.

Rather than copy the _store_lock pattern out of credentials.py — which would
be its third instance and exactly the kind of duplication that drifts — I
extracted it into codeframe/core/atomic_io.py as read_modify_write_lock().
That module is where "an atomic write is not a serialised write" belongs, it is
stdlib-only and headless, and both callers already import from it.

Same degradation as credentials.py: thread lock always, cross-process
filelock when installed, one warning when it is not so the weaker guarantee is
never silent.

2. An unreadable credential store gave a bare 500

CredentialManager's constructor runs the machine-wide migration, which can
raise CredentialStoreUnreadableError since #954. Raised from a FastAPI
dependency, it never reaches the route's own try/except, so the client got
an unformatted 500 — and lost the recovery text cf auth setup already prints.

Both routers define their own get_credential_manager and
get_credential_manager_readonly, so all four needed the guard, not the two the
issue names.

Evidence

The race tests use a threading.Barrier so both threads provably read the same
base, rather than hoping for an unlucky interleaving. Removing the lock:

FAILED  test_two_concurrent_installs_both_survive
FAILED  test_many_concurrent_installs_all_survive

and with it, 8 passed. The lock itself is tested directly too (4 threads ×
200 increments = 800, not "some number less than 800").

A test of mine was passing vacuously

test_store_raises_rather_than_clobbering wrote its corrupt ciphertext to
credentials.enc. The real file is credentials.encrypted, so the loader never
read it and the test exercised nothing — it surfaced only because the raise
assertion then failed. It uses the ENCRYPTED_FILE_NAME constant now.

Worth flagging: had I asserted something weaker, that test would have passed
while testing nothing, which is the failure mode the #954 guarantee can least
afford.

Acceptance criteria

• record_installation serialises its read-modify-write; a two-thread test records both tools
• An unreadable store returns the standard api_error(...) shape, asserted on status and body shape
• Neither path overwrites an unreadable store — asserted, ciphertext byte-identical after the raise

Full suite: 6439 passed, 49 skipped. ruff clean.

Note

read_modify_write_lock is new shared API. credentials.py still has its own
_store_lock/_get_migration_locks — I did not migrate it, because that code
is load-bearing for credential integrity and rewiring its locking is a change
that deserves its own PR rather than riding along in a P2. The duplication is
now two instances rather than three, and the new one is the documented home.

[github-actions]

Claude finished @frankbria's task in 6m 43s —— View job

---

Bug-hunt review — PR #1142

Scoped to concrete defects only (logic errors, data loss/races, security, contract breaks). Style/coverage/architecture are covered by the general review.

1 minor finding (posted inline on codeframe/ui/routers/settings_v2.py:98-102):

severity	file:line	finding
minor	codeframe/ui/routers/settings_v2.py:101 (and the 3 sibling sites in github_integrations_v2.py)	api_error(str(e), …) leaks the absolute credential-store filesystem path (e.g. /home/<op>/.codeframe/users/<id>/credentials.encrypted) into the response error field sent to an authenticated tenant — the exact host-path disclosure #934's internal_error() exists to prevent, and newly introduced where the prior bare 500 leaked nothing. Fix: log str(e) server-side, return a path-free recovery message.

Verified correct (no defect)

• record_installation serialization (installer.py:621) — the entire read-modify-write now sits inside read_modify_write_lock(...). Lock layering is thread-lock-then-file-lock (consistent ordering, no deadlock), threading.Lock()-keyed-on-Path is correct under the GIL, and degradation to thread-only when filelock is absent is honestly logged. The #954 shape guard and atomic_write_json are preserved inside the lock.
• read_modify_write_lock (atomic_io.py) — @contextmanager produces a usable CM at call sites; the 4-thread × 200-increment test genuinely exercises serialization. No reentrancy/deadlock path exists (single caller, no callback into the lock).
• Unreadable-store guards — all four CredentialManager dependency sites (both routers × write/readonly) are wrapped; no other router constructs CredentialManager as a dependency, so nothing was missed. api_error/ErrorCodes.INTERNAL_ERROR/HTTPException imports resolve.
• Vacuous-test fix (test_store_raises_rather_than_clobbering) — now writes to the real ENCRYPTED_FILE_NAME constant, so it actually exercises the loader and the byte-identical-after-raise guarantee.

Investigated and discarded (not reportable)

• clear_installation_history writes unlocked — would race a locked record_installation, but it has no production caller (only tests/core/test_installer.py), so the race is unreachable. Not a concrete failure.
• Readonly guards catch an unreachable exception — CredentialManager(migrate=False) skips migration and CredentialStore.__init__ doesn't load, so the constructor cannot raise CredentialStoreUnreadableError; the readonly try/except is harmless dead code, not a defect.
• _THREAD_LOCKS unbounded growth / double-warning race on _FILELOCK_WARNED — matches the existing credentials.py pattern; cosmetic, not a correctness defect.

No critical/major defects. The locking work is sound; the only actionable item is the path-leak above.

[claude]

Review

Reviewed the diff (atomic_io.py, installer.py, github_integrations_v2.py, settings_v2.py, and the new test file). This is a clean, well-scoped fix — verified against the current working tree (which already has this PR's commit merged in).

Correctness

• read_modify_write_lock mirrors the existing _get_migration_locks/_store_lock pattern in credentials.py closely (same thread-lock-keyed-by-Path + optional filelock degrade-with-one-warning shape), so it's a faithful extraction rather than a reinvention.
• record_installation now does its read-modify-write entirely inside the lock, with the existing malformed-history tolerance ([P2.4] Make config, credential, installer and workspace-init writes atomic and crash-safe #954) and atomic write both preserved. Looks correct.
• The four get_credential_manager/get_credential_manager_readonly dependencies (2 routers × 2 functions) are the complete set — I checked for other in-process CredentialManager(...) construction sites (github_integration_config.py, reconciliation.py, core/tasks.py) and those are called from plain function bodies (CLI / background auto-close paths), not Depends(), so they aren't subject to the "raised-from-a-dependency bypasses the route's try/except" failure mode this PR targets. The PR's claim of full coverage for that specific bug class checks out.
• api_error(str(e), ErrorCodes.INTERNAL_ERROR) matches the helper's actual signature, and the 500 + formatted-body pattern matches how every other error in these two routers is already raised (no except ... from e anywhere in the file already, so the new code isn't introducing an inconsistency there).
• The self-flagged vacuous test (credentials.enc → ENCRYPTED_FILE_NAME) is now genuinely wired to CredentialStore._load_encrypted_store / the real credentials.encrypted filename — confirmed by reading credentials.py.

Test coverage
Good: a real threading.Barrier to force the same-base-read interleaving rather than hoping for a race, a 10-thread stress variant, a direct 800-increment test of the lock itself, a no-filelock-installed degrade path, and both the formatted-500-shape and byte-identical-ciphertext-after-raise assertions the acceptance criteria call for. The TestClient(app, raise_server_exceptions=False) + bare-router pattern is already established elsewhere (test_settings_v2.py, test_settings_notifications.py, test_credential_tenant_isolation.py), so it's not a novel/fragile setup.

Minor, non-blocking nits

• settings_v2.py's new import (CredentialStoreUnreadableError before CredentialManager) breaks alphabetical order within that from codeframe.core.credentials import (...) block — cosmetic only, ruff isn't configured with isort (I) rules here so it won't fail CI, and the file's imports weren't fully sorted before this change either.
• _THREAD_LOCKS (and credentials.py's pre-existing _MIGRATION_THREAD_LOCKS) grow for the process lifetime with no eviction — bounded in practice by the number of distinct history/store directories a process touches, so not a real concern, just noting it's the same shape as the code it was extracted from.

Nothing else stood out. Scope is honest about what it's deliberately not doing (not migrating credentials.py itself onto the new helper), which matches the "don't ride along unrelated changes in a P2" guidance in this repo's CLAUDE.md.

[github-actions]

Claude finished @frankbria's task in 7m 40s —— View job

---

GLM bug-hunt review — PR #1142 (synchronize)

Re-reviewed the post-fix state (0b4ed2b + feb0bae) against origin/main. The fix commit correctly replaces the four dependency sites' api_error(str(e)) with internal_error(...) (path-free, correlation-id, recovery text appended). Scope: concrete defects only.

1 minor finding (posted inline on settings_v2.py:98):

severity	file:line	finding
minor	settings_v2.py:327 + github_integrations_v2.py:362	The path-suppression only covers the dependency (migration) raise. The same CredentialStoreUnreadableError raised from the write route body (set_credential → _load_encrypted_store) is caught by except Exception and rendered as str(e), leaking the absolute store path — the exact #934 disclosure this PR targets, on the more common path (migration is a no-op for fresh installs / already-migrated processes).

Verified correct (no defect)

• Fix commit 0b4ed2b — all four dependency guards now use internal_error(e, operation=…, logger=logger) and append the recovery text; HTTPException is already imported in both routers; body["detail"] += … is safe (internal_error always returns a fresh dict with a detail key). The no-path / correlation-id / recovery assertions in the new test genuinely cover the dependency path.
• record_installation serialization (installer.py:617) — full read-modify-write inside read_modify_write_lock(...); the #954 malformed-history tolerance and atomic_write_json are preserved inside the lock; lock path (.history.lock) is distinct from the data file.
• read_modify_write_lock (atomic_io.py) — @contextmanager yields a usable CM; thread-lock-keyed-by-Path is correct under the GIL (dict.setdefault atomic); thread-then-file ordering is consistent; FileLock-absent degrade is logged once.
• Coverage of the dependency bug class — every CredentialManager(...) in a Depends(...) resolves through one of the four guarded helpers; no other router constructs it as a dependency. The write-path guard is genuinely reachable (migration's store() raises when a machine-wide cred is copied into an unreadable per-user store).

Investigated and discarded (not reportable)

• Readonly guards catch an unreachable exception — CredentialManager(migrate=False) skips migration and CredentialStore.__init__ doesn't load the store, so the constructor cannot raise CredentialStoreUnreadableError; the readonly try/except is harmless dead code (the test forces the raise via patch, which is legitimate defensive testing of the guard itself).
• disconnect write path (github_integrations_v2.py:419) — delete_credential can raise on an unreadable store, but the route only logger.warnings and returns 204; no str(e) reaches the client, so no leak.
• _THREAD_LOCKS / _FILELOCK_WARNED unbounded growth & warning race — matches the pre-existing credentials.py pattern; cosmetic, not a correctness defect.

No critical/major defects. The locking and the dependency-guard fix are sound; the only actionable item is extending the no-leak pattern to the two write route-body catch sites above.

[claude]

Reviewed the current state of the diff (both commits). I read the two prior reviews already on this PR (the automated bug-hunt and the claude full review) — both are accurate and the path-leak finding from the bug-hunt review is genuinely fixed in the second commit (0b4ed2b), not just reworded: internal_error() now carries the message, the client only gets a correlation id + the cf auth setup recovery step, and the new test_it_does_not_leak_the_store_path / test_it_carries_a_correlation_id tests assert on the negative case directly rather than just the shape. I don't have anything to add to their correctness analysis of the locking (read_modify_write_lock, record_installation) — it holds up on a second read: thread-lock-then-optional-filelock, keyed by Path, filelock genuinely declared in pyproject.toml so the no-filelock branch is a real degrade path, not dead code.

Two things neither prior review flagged:

1. Test coverage gap: github_integrations_v2.py's fix is untested at the HTTP level.
The only CredentialStoreUnreadableError/leak test (TestAnUnreadableStoreIsAFormattedError in tests/core/test_concurrent_write_guards_1085.py) builds its TestClient against settings_v2.router only and exercises GET /api/v2/settings/keys (the readonly dependency). The identical fix was hand-duplicated into github_integrations_v2.py's get_credential_manager/get_credential_manager_readonly (and the write-path dependency in settings_v2.py), but nothing at the HTTP layer proves those three other sites don't leak the path or that they carry the correlation id — it's mechanical copy-paste, verified only by eyeballing the diff. Given how much of this PR is about a bug that survived precisely because "atomic" and "serialised" look the same, I'd apply the same skepticism here: a future edit to one site's message (e.g. tweaking the recovery text) with the other three missed would go uncaught. Not blocking, since the four blocks are currently byte-identical and the logic itself is trivial, but worth a follow-up test (or the refactor below, which would make the gap structurally impossible).

2. Minor: the four try/except blocks are now near-verbatim duplicates — worth the same treatment as read_modify_write_lock.
get_credential_manager and get_credential_manager_readonly in both routers now carry the identical ~15-line try/except (same comment, same internal_error(...) call, same appended recovery string), differing only in migrate=True vs migrate=False. The PR's own rationale for extracting read_modify_write_lock into atomic_io.py — "exactly the kind of duplication that drifts" — applies just as well here. A small shared helper (e.g. _credential_manager_or_500(auth, migrate: bool, logger) -> CredentialManager, or a decorator) would collapse four copies into one and remove the coverage gap above as a side effect. Non-blocking given the acceptance criteria are met and the duplication is currently inert, but flagging since it's the same class of "drift risk" the PR itself calls out.

No other correctness, security, or performance issues found. Scope is honest and matches the CLAUDE.md guidance about not riding unrelated changes along with a P2 fix (leaving credentials.py's own lock unmigrated).