fix(custodian): close the vulture fail-open in the pre-push gate - #493
Conversation
|
Council review: LGTM (cross-family panel, guardrail paths: src/operations_center/entrypoints/pr_review_watcher/main.py)
Council unanimous LGTM (claude_code/claude-opus-5 (correctness), claude_code/claude-opus-4-8 (security-capability), claude_code/claude-opus-4-7 (convergence-operational)). |
2 similar comments
|
Council review: LGTM (cross-family panel, guardrail paths: src/operations_center/entrypoints/pr_review_watcher/main.py)
Council unanimous LGTM (claude_code/claude-opus-5 (correctness), claude_code/claude-opus-4-8 (security-capability), claude_code/claude-opus-4-7 (convergence-operational)). |
|
Council review: LGTM (cross-family panel, guardrail paths: src/operations_center/entrypoints/pr_review_watcher/main.py)
Council unanimous LGTM (claude_code/claude-opus-5 (correctness), claude_code/claude-opus-4-8 (security-capability), claude_code/claude-opus-4-7 (convergence-operational)). |
Auto-rebase onto the base branch hit a real code conflict (beyond the union-merged journal). Manual rebase required. |
bdd6df4 to
3d59563
Compare
|
Council review: LGTM (cross-family panel, guardrail paths: src/operations_center/entrypoints/pr_review_watcher/main.py)
Council unanimous LGTM (claude_code/claude-opus-5 (correctness), claude_code/claude-opus-4-8 (security-capability), claude_code/claude-opus-4-7 (convergence-operational)). |
2 similar comments
|
Council review: LGTM (cross-family panel, guardrail paths: src/operations_center/entrypoints/pr_review_watcher/main.py)
Council unanimous LGTM (claude_code/claude-opus-5 (correctness), claude_code/claude-opus-4-8 (security-capability), claude_code/claude-opus-4-7 (convergence-operational)). |
|
Council review: LGTM (cross-family panel, guardrail paths: src/operations_center/entrypoints/pr_review_watcher/main.py)
Council unanimous LGTM (claude_code/claude-opus-5 (correctness), claude_code/claude-opus-4-8 (security-capability), claude_code/claude-opus-4-7 (convergence-operational)). |
Auto-rebase onto the base branch hit a real code conflict (beyond the union-merged journal). Manual rebase required. |
|
Council review: LGTM (cross-family panel, guardrail paths: src/operations_center/entrypoints/pr_review_watcher/main.py)
Council unanimous LGTM (claude_code/claude-opus-5 (correctness), claude_code/claude-opus-4-8 (security-capability), claude_code/claude-opus-4-7 (convergence-operational)). |
|
Needs human attention (reason= Auto-rebase onto the base branch hit a real code conflict (beyond the union-merged journal). Manual rebase required. |
The gate reported "0 findings, clean" while a Windows box running a newer Custodian reported hundreds. Windows was the correct side: the green gate was a FALSE CLEAN and had been for as long as the pin held. Three things lined up to hide it: 1. .custodian/config.yaml sets tools.vulture: true — the detector is meant to run. 2. pyproject.toml never declared vulture, so `uv pip install -e .[dev]` never installed it. The fleet venv has no vulture, and none is on PATH. 3. The pin d6ba8ab PREDATES Custodian 261bbb5 "fix(vulture): put paths before options, and stop reading a failed run as clean". On that pin the adapter built `vulture <src> --min-confidence=N <tests>`, which vulture's argparse rejects (exit 2, empty stdout) — and the empty output read as "no dead code". So even with vulture installed the pinned adapter could not emit a finding: the invocation was malformed and the failure was swallowed. The detector has never run. Bump the pin to 7a780b7 (origin/main, contains 261bbb5) and declare vulture==2.16 beside the existing ruff/ty pins. These must land together — after 261bbb5 a missing vulture fails LOUDLY, so bumping alone would red the gate on "vulture not found". Set tools.vulture_min_confidence: 80 explicitly. Custodian's adapter registry falls back to 60 while its own config loader documents 80 as the default; inheriting whichever wins is how this stays surprising. On this repo 60 yields 621 findings (essentially all UNUSED_METHOD heuristics), 80 yields 32, all at 100% confidence. Of those 32, 22 are names an external contract forces us to accept — the __exit__ protocol, pytest's pytest_sessionfinish hookspec, fixtures requested purely for a side effect, lambda stubs mirroring the callee they replace — plus two compat shims the source already documents as deliberate. Those go in a new .vulture_whitelist.py, which Custodian's adapter picks up automatically. It matches on bare NAME, not location, so it is kept minimal with a justification per entry. The remaining 10 are real and deliberately NOT whitelisted: * observer/cli.py x8 — --format, --skip-validation, --output, --filter-status, --signals-only, --input, --validate-after, --keep are declared as typer options and never read. `layers` and `full` in the same command ARE read, which is what makes these stand out rather than look like a vulture blind spot. `--format yaml` silently yields JSON. * pr_review_watcher/main.py:2508,2543 — `pending_checks` threaded through and never used. CONSEQUENCE: this turns the gate red on those 10 until they are triaged. That is the intended effect — it was green by accident. Whether each observer flag should be wired up or deleted is product work and is not guessed at here. Tracked in .console/backlog.md under Up Next. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Clears the 10 genuine findings that were holding the pre-push gate red after the vulture fail-open was closed. `custodian-multi --fail-on-findings` now exits 0 under a custodian that actually runs vulture — it reported 621 before. Corrects the earlier claim that "`layers` and `full` in the same command ARE read, so parameter-usage detection is working". That was wrong. cmd_observe_and_validate's body reads ONLY `quiet`; `layers` and `full` are equally unread there and escaped the report because vulture matches on bare NAME and those names are used by other commands in the tree. The finding was bigger than 8 stray flags: FOUR commands (observe-and-validate, compare, import, cleanup) are stubs whose entire option lists are ignored, while --help and two user guides advertised them as working. Per the "implement or delete" bar: * The four stubs are documented as PLANNED (STAGE0_CLI_SPECIFICATION.md "Secondary Commands (Planned Future)"; both guides carry "not yet implemented" notes), so deleting the commands was wrong — but keeping parameters they discard was too. Each stub now takes --quiet only. The planned interface stays in the spec, which is where a design belongs; a half-declared signature that typer advertises in --help is not a spec, it is a promise the command breaks. Dropping `import`'s required input path is deliberate: accepting a file and discarding it is indistinguishable from importing it and failing. * `list --filter valid|invalid` deleted. It could never have worked — the listing walks snapshot directories and never loads or caches a validation status to filter on (its observed_at column is a literal "—"). Implementing it needs the caching layer the help text presumed. Also fixed in cmd_cleanup, and NOT one of the vulture findings: it exited EXIT_SUCCESS while deleting nothing, so a scheduled `cleanup --days 30` reported success and silently retained every snapshot, with no way for a caller to tell a working cleanup from a stub. Now exits non-zero. Same fail-open shape as the vulture bug — a green signal that means nothing — which is why it was worth fixing in place. The guide's two runnable cleanup examples are gone and both option tables are relabelled "Planned Options (not accepted today)". Removed `pending_checks` from _update_check_history and _should_escalate_ci_wait plus 16 call sites; neither body read it. The tests passed pending_checks=["audit"] in two places, implying behaviour that could not exist — those assertions were passing for the wrong reason. test_unimplemented_stubs_reject_planned_flags pins the intent: each stub must REJECT the planned flags rather than swallow them, so an ignored option cannot be re-added without a failing test. Nothing was added to .vulture_whitelist.py — every finding was resolved by removing dead code, not by suppressing the report. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…roject The custodian-audit workflow hardcodes its own Custodian SHA, separate from pyproject's, and its comment requires the two be bumped together. This PR moved pyproject d6ba8ab -> 7a780b7 without it, so CI would have kept installing the old adapter and the vulture fail-open would have survived in the one place it matters most — the required gate. d6ba8ab predates Custodian 261bbb5, which fixed the adapter building `vulture <src> --min-confidence=N <tests>`, an argument order vulture's argparse rejects (exit 2, empty stdout) that was then read as "no dead code". That is why #492 observed "vulture was clean in CI" while vulture was installed and this repo in fact had 621 findings at the default confidence: every run failed and every failure was swallowed. It is the same vacuous-green mode the adjacent step already warns about for a missing ruff. Also drops the unpinned `pip install vulture`. vulture is a dev dependency now, so `.[dev]` pins it (2.16) beside ruff and ty — removing the moving part rather than relocating it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`.vulture_whitelist.py`, added by this branch, made `Lint (ruff)` red with 10 F821 "undefined name" errors — one per entry. The failure predates the rebase onto main; it was already recorded against this PR in the 2026-08-06 survey, and the assumption that a rebase would clear it was wrong. The errors come from the file's own content. They are also categorically wrong. Vulture matches on the bare IDENTIFIER, so a whitelist entry IS a bare name that deliberately does not resolve in that file — the whole mechanism, not an oversight. Every line trips F821 by construction, and every future entry would need `,F821` appended to its `# noqa` forever. Uses a per-file ignore in the existing [tool.ruff.lint.per-file-ignores] block instead of ten inline suppressions: one statement of intent, nothing to remember when adding an entry. Scoped to the single file, and verified scoped — injecting a real undefined name into src/operations_center/injection.py still reports F821, so the gate is not widened. `ruff check .` now passes repo-wide. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This branch was unpushable: .console/log.md exceeded OC2's 500KB budget, because main's log sits at 98% of it and OC's pre-commit hook requires every PR to add an entry. #498 carries the rotation already, but waiting on it serialises the whole PR queue behind a GitHub outage. The archive is copied verbatim from #498's branch, so every branch carries an identical docs/history/console-log/log-archive-through-2026-06-14.md; whichever merges first, the rest rebase onto an already-applied change instead of conflicting. Entries to drop are matched by heading with multiplicity, not by whole-entry string equality. Two earlier approaches aborted on their own safety checks: the archive is not a clean suffix of main's log (log.md is not consistently newest-first), and string equality reported phantom "unaccounted" entries because the last entry of a slice absorbs the trailing content after it. A heading census confirms the split is lossless — 0 of main's 294 headings are missing from archive+kept. The inherited filename is inaccurate (the block spans 2026-06-04 to 2026-07-14, not "through 2026-06-14" — the split was by size, not date). Kept anyway: renaming would diverge from #498 and cause the conflict this avoids. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
d37642b to
10b3f74
Compare
|
Council review: LGTM (cross-family panel, guardrail paths: src/operations_center/entrypoints/pr_review_watcher/main.py)
Council unanimous LGTM (claude_code/claude-opus-5 (correctness), claude_code/claude-opus-4-8 (security-capability), claude_code/claude-opus-4-7 (convergence-operational)). |
1 participant
The gate was lying
The Custodian pre-push gate reported
0 findings, cleanon this repo, while a Windows box running a newer Custodian reported hundreds. Windows was the correct side. The green gate was a false clean, and had been for as long as the pin has been in place.Three things had to line up to hide it:
.custodian/config.yamlsetstools.vulture: true— the detector is meant to run.pyproject.tomlnever declaredvulturein the dev extra, souv pip install -e .[dev]never installed it. The fleet venv has no vulture and none is on PATH.d6ba8abpredates Custodian261bbb5— "fix(vulture): put paths before options, and stop reading a failed run as clean". On that pin the adapter builtvulture <src> --min-confidence=N <tests>, which vulture's argparse rejects (exit 2, empty stdout), and the empty output was read as "no dead code".So even had vulture been installed, the pinned adapter could not have produced a finding — the invocation itself was malformed and the failure was swallowed. The detector has never once run.
The fix
7a780b7(origin/main, contains261bbb5)vulture==2.16alongside the existingruff/typinsThese must land together: after
261bbb5a missing vulture fails loudly, so bumping the pin alone would red the gate on "vulture not found".Threshold set explicitly to
tools.vulture_min_confidence: 80. Custodian's adapter registry falls back to 60 while its own config loader documents 80 as the intended default — relying on whichever wins is how this stays surprising.--min-confidenceUNUSED_METHOD/attribute heuristics)What's whitelisted, and what deliberately isn't
Of the 32, 22 are names an external contract forces us to accept: the
__exit__protocol, pytest'spytest_sessionfinishhookspec, fixtures requested purely for a side effect, and lambda stubs that must mirror the callee they replace — plus two compat shims the source already documents as deliberate (max_rewrite_attemptscarries# noqa: ARG002 — kept for signature compat;queue_thresholdcarries# kept for config compat, not used in logic).Those go in a new
.vulture_whitelist.py, which Custodian's adapter picks up automatically when present. It matches on bare name, not location, so it's kept minimal with a justification per entry.The remaining 10 are real and are deliberately not whitelisted:
observer/cli.py×8 —--format,--skip-validation,--output,--filter-status,--signals-only,--input,--validate-after,--keepare declared astyper.Option(...)and never read in the body.layersandfullin the same command are read, which is what makes these stand out rather than look like a vulture blind spot. User-visible: passing--format yamltoday silently produces JSON.pr_review_watcher/main.py:2508,2543—pending_checksthreaded through two call sites and never used.Consequence
Merging turns the gate red until those 10 are triaged. Whether each observer flag should be wired up or deleted is product work and is not guessed at here — tracked in
.console/backlog.mdunder "Up Next".If you'd rather not take a red gate right now, hold this PR and I'll fix the 10 first; the fail-open has been in place a long time and a few more days won't change anything.
Separate finding, not fixable from this repo
5ef3f0f fix(adapters): make find_tool's venv-first preference work on Windowsexists only in the local Custodian checkout (branchclaude/reconcile-june-2026-08-03, upstream gone).origin/mainis at7a780b7, so that fix can't be pinned. Until it's pushed, a Windows Custodian run resolves linters off PATH rather than a venv — that produced 1222 phantom ruff findings earlier today (ruff 0.16's widened default rule set vs OC's pinned 0.15.13) until the local checkout picked the commit up mid-session. Also tracked in the backlog.Verification
The 10 are exactly the genuine set listed above. Every whitelisted name was inspected at its source location and classified before being added.
🤖 Generated with Claude Code