From 5c702c544d3694854064dcb64c21da78737c242a Mon Sep 17 00:00:00 2001 From: Kyzcreig <9063726+Kyzcreig@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:03:29 -0700 Subject: [PATCH] fix(kanban): log git's returncode and stderr when survivor capture fails `_git` raised a constant `survivor_unavailable: git inspection failed` and discarded the returncode and stderr, so a real capture defect and a purely environmental fault were indistinguishable at the call site. Establishing which cost a full attribution pass per occurrence (t_ac0e595c), and kanban_survivor.py has five open PRs whose reviewers each pay that tax. Root cause of the occurrences that prompted this (card t_169d6e46): NOT a survivor defect and not a RAM-disk race. fleet/ramscratch-env.sh exported a FIXED `--basetemp=/Volumes/ramscratch/pytest` into every daedalus-family worker shell; pytest rm_rf()s an explicit --basetemp at session start with no numbered subdir and no lock, so concurrent workers deleted each other's live tmp_path and git exited 128 "cannot change to '': No such file or directory". Measured with 2 concurrent sessions of the survivor suite: 10/12 sessions red with the fixed basetemp, 0/18 red without it. The env fix lands separately in hermes-home; this commit is what makes the next occurrence diagnosable in one log line instead of an attribution pass. The raised message is unchanged (it is persisted to held_reason, the event log and stderr, and open PRs key on it). Detail goes to the log, redacted through kanban_external_survivor.redact, the same helper that guards an echoed claim. Verified: - 2 new tests in test_kanban_survivor_authority.py, born-red 2/2 against unmodified fork/main (assert 'rc=128' in '' / IndexError on no records) - teeth: mutating the log to unredacted stderr kills ONLY the redaction test (1 failed, 8 passed), so it is not a vacuous green - 74/74 x3 across all four survivor test files - ruff clean on both changed files --- hermes_cli/kanban_survivor.py | 16 +++++- .../test_kanban_survivor_authority.py | 51 +++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/hermes_cli/kanban_survivor.py b/hermes_cli/kanban_survivor.py index a357dedbf165..278915a36118 100644 --- a/hermes_cli/kanban_survivor.py +++ b/hermes_cli/kanban_survivor.py @@ -34,7 +34,21 @@ def _git(repo, *args, env=None, check=True): capture_output=True, timeout=30, env=env, ) if check and result.returncode: - # Git stderr can contain credential-bearing remote URLs. Do not persist it. + # Git stderr can contain credential-bearing remote URLs, so it is never + # persisted: the raised message stays constant and the detail goes to + # the log, redacted by the same helper that guards an echoed claim. + # + # Without this, every occurrence costs an attribution pass. A failure + # that is purely environmental (t_169d6e46: a concurrent pytest session + # deleting this repo's tmp_path, so git exits 128 "cannot change to + # '': No such file or directory") is indistinguishable from a real + # capture defect once it reaches the caller as a bare + # "survivor_unavailable: git inspection failed". + log.warning( + "kanban survivor: git %s failed rc=%s in %s: %s", + args[0] if args else "?", result.returncode, _ext.redact(str(repo)), + _ext.redact(result.stderr.decode("utf-8", "replace").strip()), + ) raise SurvivorUnavailable("survivor_unavailable: git inspection failed") return result diff --git a/tests/hermes_cli/test_kanban_survivor_authority.py b/tests/hermes_cli/test_kanban_survivor_authority.py index 5be52cfaafb0..e9f5b5e3d403 100644 --- a/tests/hermes_cli/test_kanban_survivor_authority.py +++ b/tests/hermes_cli/test_kanban_survivor_authority.py @@ -205,3 +205,54 @@ def test_mined_pr_corroborated_by_the_cards_own_metadata_is_accepted(board, remo saved = kb.latest_run(board, tid).metadata["survivor"] assert saved["kind"] == "ref" assert saved["refs"][0]["sha"] == MERGE + + +# --- t_169d6e46: a swallowed git failure must be diagnosable ---------------- + +def test_git_failure_logs_returncode_and_stderr_without_persisting_them(tmp_path, caplog): + """`survivor_unavailable: git inspection failed` discards WHY it failed. + + That message is identical for a real capture defect and for a purely + environmental fault (t_169d6e46: a concurrent pytest session deleting this + repo's tmp_path, so git exits 128 `cannot change to ''`). Telling them + apart cost a full attribution pass per occurrence. The returncode and stderr + must reach the LOG; the raised message must stay constant and credential-free, + because it is persisted to held_reason, the event log and stderr. + """ + import hermes_cli.kanban_survivor as survivor + + missing = tmp_path / "vanished" # never created: git exits 128 + with caplog.at_level("WARNING"): + with pytest.raises(survivor.SurvivorUnavailable) as excinfo: + survivor._git(missing, "status", "--porcelain") + + # The persisted surface is unchanged — 5 open PRs key on this string. + assert str(excinfo.value) == "survivor_unavailable: git inspection failed" + # ...and the diagnostic that distinguishes environment from defect is logged. + assert "rc=128" in caplog.text + assert "status" in caplog.text + assert "No such file or directory" in caplog.text + + +def test_git_failure_log_redacts_credentials_from_stderr(tmp_path, caplog, monkeypatch): + """Git stderr can carry a credential-bearing remote URL; the log must not. + + Teeth for the test above: an unredacted `log.warning(stderr)` would satisfy + every assertion there while leaking a PAT into the logfile. + """ + import subprocess as sp + + import hermes_cli.kanban_survivor as survivor + + leaky = f"fatal: could not read from https://x-access-token:{SECRET}@github.com/a/b.git\n" + monkeypatch.setattr( + survivor.subprocess, "run", + lambda *a, **k: sp.CompletedProcess(a[0], 128, b"", leaky.encode()), + ) + with caplog.at_level("WARNING"): + with pytest.raises(survivor.SurvivorUnavailable): + survivor._git(tmp_path, "fetch") + + assert SECRET not in caplog.text + assert SECRET not in str(caplog.records[-1].getMessage()) + assert "rc=128" in caplog.text # still diagnosable