diff --git a/hermes_cli/kanban_survivor.py b/hermes_cli/kanban_survivor.py index a357dedbf1659..278915a361183 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 5be52cfaafb0d..e9f5b5e3d4033 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