fix(adapters): find_tool must prefer the AUDITED repo's venv, not Custodian's - #72
Merged
Merged
Conversation
…todian's
A globally-installed `custodian-multi` audited OperationsCenter — which pins
`ruff==0.15.13` — using a system-wide ruff 0.16.1, and reported 1222 findings
against a tree OC's own `ruff check` calls clean (BLE001 x316, UP045 x290,
UP037 x118, ...). Same repo, same config, ruff 0.15.13: 0 findings. Vulture
skewed the same way (621 findings). All of them phantom.
Root cause: `find_tool()` resolved `Path(sys.executable).parent / name` — the
venv Custodian ITSELF is installed in — then fell back to PATH. For a
multi-repo auditor that is backwards. Each repo pins the toolchain its config
was written against, so an audit is only meaningful when it runs those
versions; the venv Custodian happens to live in has no authority over the repo
in front of it. The old order was right by accident in the single-repo case
(Custodian installed into the audited repo's venv) and silently wrong
everywhere else — and "silently" is the problem, since the output is a
plausible-looking wall of real rule codes.
A second bug hid in the same three lines: `Path(sys.executable).parent / name`
can never match on Windows, where console scripts are `ruff.exe` and live in
`Scripts/` rather than `bin/`. The venv branch was dead code on that platform.
Resolution order is now:
1. the audited repo's own venv (`.venv` then `venv`, `bin/` or `Scripts/`)
2. Custodian's own venv (unchanged fallback, still needed when the venv is
not fully activated and `shutil.which` alone would miss it)
3. PATH
`_executable()` tries `.exe`/`.bat`/`.cmd` on Windows, and both script-dir
spellings are accepted on either host — a venv built under WSL but audited
from Windows over /mnt/c carries the other platform's layout.
The audited repo is scoped with a ContextVar and an `audited_repo()` context
manager rather than a new parameter: `is_available()` takes no arguments, and
it and `run()` must agree on which binary they are talking about.
`cli/runner._run_adapters` wraps its adapter loop in it.
Verified: 1238 passed, 5 skipped. Six new tests cover repo-venv preference, the
no-venv fallback, both script-dir spellings, and that the ContextVar does not
leak past the loop — a leak would make later repos in a `--repos a b c` run
inherit the first repo's toolchain. Custodian's own audit is unchanged from
baseline; the one remaining finding (W2, core.hooksPath unset) is
environmental. Live proof: under `audited_repo(~/GitHub/OperationsCenter)`,
`find_tool('ruff')` returns OC's pinned `.venv/bin/ruff` rather than
Custodian's own.
Noted, not fixed (pre-existing, reproduces at origin/main): tests/test_reconcile.py
does not isolate $REPOGRAPH_BOUNDARY_ARTIFACT_FILE, so two tests fail whenever
that variable is set in the caller's environment.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The 2026-08-03 reconciliation (#71) landed log.md at 395 lines against a 400-line r1_line_budget, leaving no room for the next entry to be written at all — the find_tool entry took it to 437 and red-failed our own audit on RC1. Second pass, same convention as the first: prune the two oldest surviving entries (2026-06-20 INJ1 detector, 2026-07-10 C32 punctuation-only values) into the reconciled note, and condense the find_tool entry to a summary. Full rationale lives in the commit message and PR #72; full history in git as always. 396 lines. Custodian's own audit is back to baseline — the single remaining finding (W2, core.hooksPath unset) is environmental in this clone and does not occur in CI, which sets it as the audit job's first step. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ProtocolWarden
added a commit
to ProtocolWarden/OperationsCenter
that referenced
this pull request
Aug 4, 2026
…or a week (#492) CI has failed on main every day since at least 2026-07-29. Both lint gates installed ruff UNPINNED while the repo pins ruff==0.15.13: ci.yml pip install "ruff>=0.5" -> floated to 0.16.1 custodian-audit.yml pip install ruff vulture ty -> same drift `ruff check .` went from clean to 1996 errors; the Custodian audit reported 1222 findings (the ruff group alone — vulture was clean in CI). None of them real. [tool.ruff.lint] selects a deliberate rule set, and its own comment records BLE001 and S110 as DROPPED — "too noisy across codebase, real legitimate uses". A newer ruff re-enables exactly those: of the 1222, BLE001 accounted for 316 and UP045 for 290. Verified on the same tree: ruff 0.16.1 -> 1222 findings, ruff 0.15.13 -> "All checks passed!" across the full `ruff check .`, root files included. The tree was never dirty. Both jobs now install `-e ".[dev]"`, so the version comes from [project.optional-dependencies].dev. One source of truth, and no version literal left in the workflows to drift again. custodian-audit.yml already carried a paragraph explaining that Custodian itself must be SHA-pinned, because tracking @main once let an upstream change emit "a phantom finding fleet-wide". The next line then installed that pinned auditor's TOOLS unpinned and reproduced the same failure one level down. Pinning the auditor while floating what the auditor runs pins nothing. Also drops `|| true` from the repo install. Best-effort was actively harmful here: on failure the adapters find no ruff, Custodian reports it "not installed" and skips it, and the gate passes vacuously — a green check that audited nothing. Same root cause one layer up, fixed separately in ProtocolWarden/Custodian#72: find_tool() preferred Custodian's own venv over the audited repo's, so a globally-installed custodian-multi reproduced this identically off-CI. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
A globally-installed
custodian-multiaudited OperationsCenter — which pinsruff==0.15.13— using a system-wide ruff 0.16.1, and reported 1222 findings against a tree OC's ownruff checkcalls clean (BLE001×316,UP045×290,UP037×118, …). Same repo, same.custodian/config.yaml, ruff 0.15.13: 0 findings. Vulture skewed the same way (621). Every one of them was phantom.This is what turned OC's pre-push gate into a wall that no push could clear, on a baseline that is in fact clean.
Root cause
find_tool()resolvedPath(sys.executable).parent / name— the venv Custodian itself is installed in — then fell back to PATH.For a multi-repo auditor that is backwards. Each repo pins the toolchain its config was written against, so an audit is only meaningful when it runs those versions; the venv Custodian happens to live in has no authority over the repo in front of it. The old order was right by accident in the single-repo case (Custodian installed into the audited repo's venv) and silently wrong everywhere else — and silently is the problem, since the output is a plausible-looking wall of real rule codes.
A second bug hid in the same three lines:
Path(sys.executable).parent / namecan never match on Windows, where console scripts areruff.exeand live inScripts/rather thanbin/. The venv branch was dead code on that platform.The fix
Resolution order is now:
.venvthenvenv;bin/orScripts/)shutil.whichalone would miss it_executable()tries.exe/.bat/.cmdon Windows, and both script-dir spellings are accepted on either host: a venv built under WSL but audited from Windows over/mnt/ccarries the other platform's layout.The audited repo is scoped with a
ContextVar+audited_repo()context manager rather than a new parameter —is_available()takes no arguments, and it andrun()must agree on which binary they are talking about.cli/runner._run_adapterswraps its adapter loop in it.Verification
--repos a b crun inherit the first repo's toolchain.W2,core.hooksPathunset) is environmental and pre-existing.audited_repo(~/GitHub/OperationsCenter),find_tool('ruff')returns OC's pinned.venv/bin/ruffinstead of Custodian's own.Noted, not fixed
Pre-existing and reproduces at
origin/main:tests/test_reconcile.pydoes not isolate$REPOGRAPH_BOUNDARY_ARTIFACT_FILE, so two tests fail whenever that variable is set in the caller's environment. Out of scope here.🤖 Generated with Claude Code