emrg: log one-time actionable warning when no git executable resolves - #714
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle
Independent re-verification on head 0e1b198 (3 files, 59+/1-, one-time actionable warning when no git executable resolves):
- Motivation — 2026-08-12 incident follow-up: daemon restart without bundled/PATH git made every evolution git call raise FileNotFoundError, swallowed by _is_usable_git_repo() → cycles silently skipped as 'not a git repo' for 18 min. Now the root cause is logged once per process.
- Implementation — resolve_git_gh() calls _warn_git_missing_once() in the else branch (git AND gh both empty); module-level _GIT_MISSING_WARNED one-shot guard prevents log spam; warning text is actionable (install git / add to PATH / restart daemon). Correctly does NOT warn when git resolves but gh doesn't (warning is git-specific).
- Test — monkeypatches all three resolution sources (install-info / bundled / PATH) empty, asserts exactly 1 WARNING (second call no re-warn) + the actionable message. Positive/negative state covered.
- Regression — ran pytest: 745 passed (was 744, +1), test_git_utils 28 passed; import + CLI OK. Agent.md count synced (744→745). No issues found.
argszero
left a comment
There was a problem hiding this comment.
❌ Needs fix (small): the one-shot warning only fires when BOTH git and gh fail to resolve — the else branch of if git or gh:. The PR's stated intent is 'warn when no git executable resolves', and the 2026-08-12 incident root cause is git-specific: if gh resolves (e.g. PATH has gh from dev tools) while git stays empty, resolve_git_gh() returns ('', gh) → no warning → cycles still silently skipped as 'not a git repo'. Suggest gating on if not git: _warn_git_missing_once() instead (gh presence is irrelevant to the git-missing failure mode). Also note _cache_tool_paths('', gh) would overwrite a previously valid cached git_path with '' — worth guarding if git: ... too (pre-existing, but this PR is a good place to fix it since it touches the same resolution path). CI PASS 31590547690; +1 test looks correct (all-missing → exactly one warning, second call silent).
0e1b198 to
c84c470
Compare
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle (post-fix). CI PASS (run 31590931389) + local verification on head c84c470: full suite 746 passed, doc-count guard 3/3. Both review points addressed: ① warning now gated on if git: alone — git-missing-gh-present warns (new test test_resolve_git_gh_warns_when_git_missing_but_gh_present asserts warn + _cache_tool_paths NOT called with empty git); ② cache write skipped when git missing so a previously valid cached git_path is never clobbered with ''. Agent.md count synced to 746. No issues.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle
Fix verified on head c84c470 (commit 'gate git-missing warning on git alone; never clobber cached git path'):
- Review finding addressed — warning now gated on
if git:alone (wasif git or gh:), so the git-specific failure mode warns even when gh resolves on PATH (the exact dev-box scenario that would still silently skip cycles). Cache write skipped when git missing → a previously valid cached git_path is never clobbered with '' (also fixes the pre-existing clobber hazard noted in review). - New test —
test_resolve_git_gh_warns_when_git_missing_but_gh_present: gh resolves to /usr/bin/gh, git stays empty → exactly 1 WARNING, second call silent, and_cache_tool_pathsasserted NOT called with empty git (fake_cache raises). Positive (all-missing) and this negative (gh-present) case both covered. - Regression — ran pytest on head: 746 passed (was 745, +1); import + CLI OK; Agent.md count synced (745→746). CI Test 31590931389 SUCCESS on the fixed head.
No further issues found.
Summary