What happened
During a refactoring to make tool ordering a single source of truth, an intermediate derived list (OTHER_TOOLS) was introduced in one commit, then made redundant in the next commit when its only consumer was replaced by a more direct approach (ALL_TOOL_SPECS). The list and its test were not removed, and the oversight was only caught in code review.
The test for OTHER_TOOLS was circular: it asserted that the derived list equalled the same filter expression used to compute it. This tested nothing real — if the derivation was wrong, both sides of the assertion would be equally wrong.
Root cause
A multi-step refactoring removed all real consumers of an intermediate derived value, but no final audit pass checked whether earlier work in the same PR had become redundant. There is no automated dead-export detection to flag unused module-level constants.
Generalised principle
After any refactoring, scan for intermediate derived values (especially derived lists or filtered collections) introduced earlier in the same PR that may have become redundant. A test that asserts a derived value equals the expression it was derived from is circular — it validates the derivation, not the value. When all real consumers of a derived value are gone, remove both the value and its circular test. A useful heuristic: if the only test of a value is assert derived == [x for x in source if condition], the test is telling you the value should not exist.
Resolution
OTHER_TOOLS and the TYPE_CHECKING/Tool import that existed solely to annotate it were removed from all_.py. The circular TestOtherTools test class was deleted. The one meaningful invariant (that ALL_TOOL_SPECS stays in sync with ALL_TOOLS) was retained under a renamed TestAllToolSpecs class.
Follow-up
Consider whether a static check (e.g. a custom ruff rule or import-linter contract) could detect unused public module-level constants in future.
What happened
During a refactoring to make tool ordering a single source of truth, an intermediate derived list (
OTHER_TOOLS) was introduced in one commit, then made redundant in the next commit when its only consumer was replaced by a more direct approach (ALL_TOOL_SPECS). The list and its test were not removed, and the oversight was only caught in code review.The test for
OTHER_TOOLSwas circular: it asserted that the derived list equalled the same filter expression used to compute it. This tested nothing real — if the derivation was wrong, both sides of the assertion would be equally wrong.Root cause
A multi-step refactoring removed all real consumers of an intermediate derived value, but no final audit pass checked whether earlier work in the same PR had become redundant. There is no automated dead-export detection to flag unused module-level constants.
Generalised principle
After any refactoring, scan for intermediate derived values (especially derived lists or filtered collections) introduced earlier in the same PR that may have become redundant. A test that asserts a derived value equals the expression it was derived from is circular — it validates the derivation, not the value. When all real consumers of a derived value are gone, remove both the value and its circular test. A useful heuristic: if the only test of a value is
assert derived == [x for x in source if condition], the test is telling you the value should not exist.Resolution
OTHER_TOOLSand theTYPE_CHECKING/Toolimport that existed solely to annotate it were removed fromall_.py. The circularTestOtherToolstest class was deleted. The one meaningful invariant (thatALL_TOOL_SPECSstays in sync withALL_TOOLS) was retained under a renamedTestAllToolSpecsclass.Follow-up
Consider whether a static check (e.g. a custom ruff rule or import-linter contract) could detect unused public module-level constants in future.