diff --git a/src/vouch/admission.py b/src/vouch/admission.py index 207d4992..b7e3abba 100644 --- a/src/vouch/admission.py +++ b/src/vouch/admission.py @@ -42,6 +42,8 @@ import yaml +from .config_coerce import coerce_bool + if TYPE_CHECKING: from .storage import KBStore @@ -103,10 +105,11 @@ def load_config(store: KBStore) -> AdmissionConfig: if not isinstance(raw, dict): return AdmissionConfig() return AdmissionConfig( - enabled=bool(raw.get("enabled", DEFAULT_ENABLED)), + enabled=coerce_bool(raw.get("enabled", DEFAULT_ENABLED), DEFAULT_ENABLED), min_confidence=_as_float(raw.get("min_confidence")) or DEFAULT_MIN_CONFIDENCE, - reject_uncited_session_pages=bool( - raw.get("reject_uncited_session_pages", DEFAULT_REJECT_UNCITED_SESSION_PAGES) + reject_uncited_session_pages=coerce_bool( + raw.get("reject_uncited_session_pages", DEFAULT_REJECT_UNCITED_SESSION_PAGES), + DEFAULT_REJECT_UNCITED_SESSION_PAGES, ), ) diff --git a/src/vouch/capture.py b/src/vouch/capture.py index 0b6b05c1..2c3aef1a 100644 --- a/src/vouch/capture.py +++ b/src/vouch/capture.py @@ -31,6 +31,7 @@ import yaml +from .config_coerce import coerce_bool from .enrich import Enrichment from .models import ProposalStatus from .secrets import mask_secrets @@ -70,7 +71,7 @@ def load_config(store: KBStore) -> CaptureConfig: if answer_mode not in _ANSWER_MODES: answer_mode = DEFAULT_ANSWER_MODE return CaptureConfig( - enabled=bool(raw.get("enabled", DEFAULT_ENABLED)), + enabled=coerce_bool(raw.get("enabled", DEFAULT_ENABLED), DEFAULT_ENABLED), min_observations=int(raw.get("min_observations", DEFAULT_MIN_OBSERVATIONS)), dedup_window_seconds=float( raw.get("dedup_window_seconds", DEFAULT_DEDUP_WINDOW_SECONDS) diff --git a/src/vouch/compile.py b/src/vouch/compile.py index d70421b6..2d5ce31c 100644 --- a/src/vouch/compile.py +++ b/src/vouch/compile.py @@ -29,6 +29,7 @@ from . import audit as audit_mod from . import llm_draft +from .config_coerce import coerce_bool from .context import _RETRACTED_CLAIM_STATUSES from .models import ProposalStatus from .proposals import ProposalError, _slugify, propose_page @@ -169,7 +170,7 @@ def load_config(store: KBStore) -> CompileConfig: raw.get("timeout_seconds", DEFAULT_TIMEOUT_SECONDS), DEFAULT_TIMEOUT_SECONDS, float, ), - two_phase=bool(raw.get("two_phase", False)), + two_phase=coerce_bool(raw.get("two_phase", False), False), ) diff --git a/src/vouch/config_coerce.py b/src/vouch/config_coerce.py new file mode 100644 index 00000000..0e2719a7 --- /dev/null +++ b/src/vouch/config_coerce.py @@ -0,0 +1,39 @@ +"""shared config-boolean coercion. + +`bool(x)` on an arbitrary config value is a trap: `yaml.safe_load` resolves +an *unquoted* `true`/`false` to a real Python bool, but a mistakenly-quoted +`"false"` in config.yaml stays a non-empty string, and `bool("false")` is +True. A dozen `load_config()` functions across this codebase each read a +boolean flag out of raw YAML; before this module they either repeated the +same `bool(raw.get(...))` bug (compile.py's `two_phase`, and the `enabled` +flag in session_split/admission/inbox/recall/capture/volunteer_context) or +raised hard on a bad value (install_adapter.py's install.yaml flags, which +is the right call for a one-time CLI install but not for config read on +every session/render). + +`coerce_bool()` is the fail-soft half of that spectrum: recognized string +spellings are parsed case-insensitively, anything else (wrong type, typo, +unrecognized spelling) degrades to the caller's default rather than taking +down every caller on a config mistake -- the same fail-soft posture +compile.py's own `_coerce()` already documents for its numeric fields. +""" + +from __future__ import annotations + +from typing import Any + +_TRUE_STRINGS = frozenset({"true", "yes", "on", "1"}) +_FALSE_STRINGS = frozenset({"false", "no", "off", "0"}) + + +def coerce_bool(value: Any, default: bool) -> bool: + """parse a YAML-sourced config boolean, defaulting on anything unrecognized.""" + if isinstance(value, bool): + return value + if isinstance(value, str): + lowered = value.strip().lower() + if lowered in _TRUE_STRINGS: + return True + if lowered in _FALSE_STRINGS: + return False + return default diff --git a/src/vouch/inbox.py b/src/vouch/inbox.py index 82ee365b..963859a4 100644 --- a/src/vouch/inbox.py +++ b/src/vouch/inbox.py @@ -23,6 +23,7 @@ import yaml +from .config_coerce import coerce_bool from .proposals import propose_page from .storage import KBStore @@ -62,7 +63,7 @@ def load_config(store: KBStore) -> InboxConfig: return InboxConfig() extensions = raw.get("extensions") return InboxConfig( - enabled=bool(raw.get("enabled", True)), + enabled=coerce_bool(raw.get("enabled", True), True), min_chars=int(raw.get("min_chars", DEFAULT_MIN_CHARS)), extensions=( tuple(str(e) for e in extensions) diff --git a/src/vouch/proposals.py b/src/vouch/proposals.py index 2c8feece..dd64bf4d 100644 --- a/src/vouch/proposals.py +++ b/src/vouch/proposals.py @@ -18,6 +18,7 @@ from pydantic import ValidationError from . import admission, audit, index_db +from .config_coerce import coerce_bool from .models import ( ArtifactScope, Claim, @@ -513,7 +514,16 @@ def propose_delete( def _review_config(store: KBStore) -> dict[str, Any]: - """The ``review:`` section of config.yaml, or {} if absent/unreadable.""" + """the ``review:`` section of config.yaml, or {} if absent/unreadable. + + ``auto_approve_on_receipt`` is normalized to a real bool here -- the + single source of truth every caller below reads from -- so a + mistakenly-quoted ``auto_approve_on_receipt: "false"`` in config.yaml + can never be silently treated as enabled by one call site while another + (correctly) treats the same value as disabled. callers that still wrap + this in their own ``bool(...)`` are unaffected: bool() on an already-real + bool is a no-op. + """ try: loaded = yaml.safe_load( (store.kb_dir / "config.yaml").read_text(encoding="utf-8") @@ -521,7 +531,11 @@ def _review_config(store: KBStore) -> dict[str, Any]: except Exception: return {} if isinstance(loaded, dict) and isinstance(loaded.get("review"), dict): - return loaded["review"] + review = dict(loaded["review"]) + review["auto_approve_on_receipt"] = coerce_bool( + review.get("auto_approve_on_receipt"), False + ) + return review return {} diff --git a/src/vouch/recall.py b/src/vouch/recall.py index b05b0581..8195c3f1 100644 --- a/src/vouch/recall.py +++ b/src/vouch/recall.py @@ -16,6 +16,7 @@ import yaml +from .config_coerce import coerce_bool from .context import _RETRACTED_CLAIM_STATUSES from .scoping import ViewerContext, is_visible, viewer_from from .storage import KBStore @@ -45,7 +46,7 @@ def load_config(store: KBStore) -> RecallConfig: if not isinstance(raw, dict): return RecallConfig() return RecallConfig( - enabled=bool(raw.get("enabled", DEFAULT_ENABLED)), + enabled=coerce_bool(raw.get("enabled", DEFAULT_ENABLED), DEFAULT_ENABLED), max_chars=int(raw.get("max_chars", DEFAULT_MAX_CHARS)), ) diff --git a/src/vouch/session_split.py b/src/vouch/session_split.py index 01ecd5df..f8b042fa 100644 --- a/src/vouch/session_split.py +++ b/src/vouch/session_split.py @@ -21,6 +21,7 @@ from . import audit as audit_mod from . import capture, enrich, llm_draft from . import compile as compile_mod +from .config_coerce import coerce_bool from .llm_draft import LLMDraftError from .models import ProposalStatus from .proposals import _slugify, propose_page, reject @@ -71,7 +72,7 @@ def load_split_config(store: KBStore) -> SplitConfig: return SplitConfig() llm_cmd = raw.get("llm_cmd") return SplitConfig( - enabled=bool(raw.get("enabled", True)), + enabled=coerce_bool(raw.get("enabled", True), True), llm_cmd=str(llm_cmd) if llm_cmd else None, threshold_observations=_coerce( raw.get("threshold_observations", DEFAULT_THRESHOLD_OBSERVATIONS), diff --git a/src/vouch/volunteer_context.py b/src/vouch/volunteer_context.py index 33fd23c4..a4b9355d 100644 --- a/src/vouch/volunteer_context.py +++ b/src/vouch/volunteer_context.py @@ -17,6 +17,7 @@ import yaml from . import hot_memory +from .config_coerce import coerce_bool from .context import _RETRACTED_CLAIM_STATUSES from .models import Session from .scoping import ViewerContext, viewer_from @@ -74,7 +75,7 @@ def load_config(store: KBStore) -> VolunteerConfig: raw = loaded.get("volunteer") if not isinstance(raw, dict): return VolunteerConfig() - enabled = bool(raw.get("enabled", True)) + enabled = coerce_bool(raw.get("enabled", True), True) threshold = float(raw.get("threshold", DEFAULT_THRESHOLD)) throttle = float(raw.get("throttle_seconds", DEFAULT_THROTTLE_SECONDS)) poll = float(raw.get("poll_interval_seconds", DEFAULT_POLL_INTERVAL)) diff --git a/tests/test_admission.py b/tests/test_admission.py index abc4f96c..ac1649ad 100644 --- a/tests/test_admission.py +++ b/tests/test_admission.py @@ -50,6 +50,21 @@ def store(tmp_path, monkeypatch) -> KBStore: return s +def test_load_config_quoted_false_does_not_enable(store: KBStore) -> None: + """Regression: bool("false") is True in plain Python, so a mistakenly + quoted `enabled: "false"` previously silently kept admission gating on, + and `reject_uncited_session_pages: "false"` kept that rule on too.""" + store.config_path.write_text( + store.config_path.read_text(encoding="utf-8") + + '\nadmission:\n enabled: "false"\n' + ' reject_uncited_session_pages: "false"\n', + encoding="utf-8", + ) + cfg = admission.load_config(store) + assert cfg.enabled is False + assert cfg.reject_uncited_session_pages is False + + # ---------------------------------------------------------------- pure predicate @pytest.mark.parametrize("text", FRAGMENT_CLAIMS) def test_assess_claim_rejects_structural_fragments(text: str) -> None: diff --git a/tests/test_capture.py b/tests/test_capture.py index a6b0a4aa..e3de363f 100644 --- a/tests/test_capture.py +++ b/tests/test_capture.py @@ -31,6 +31,13 @@ def test_load_config_reads_override(store: KBStore) -> None: assert cfg.min_observations == 5 +def test_load_config_quoted_false_does_not_enable(store: KBStore) -> None: + """Regression: bool("false") is True in plain Python, so a mistakenly + quoted `enabled: "false"` previously silently kept capture enabled.""" + store.config_path.write_text('capture:\n enabled: "false"\n') + assert cap.load_config(store).enabled is False + + def test_buffer_path_under_captures_dir(store: KBStore) -> None: p = cap.buffer_path(store, "sess-123") assert p == store.kb_dir / "captures" / "sess-123.jsonl" diff --git a/tests/test_compile.py b/tests/test_compile.py index f4650d07..c6d70b76 100644 --- a/tests/test_compile.py +++ b/tests/test_compile.py @@ -639,6 +639,18 @@ def test_two_phase_config_flag_parsed(store: KBStore) -> None: assert cfg.two_phase is True +def test_two_phase_quoted_false_string_does_not_enable(store: KBStore) -> None: + """Regression: bool("false") is True in plain Python, so a mistakenly + quoted `two_phase: "false"` previously silently enabled two-phase mode.""" + store.config_path.write_text( + store.config_path.read_text(encoding="utf-8") + + '\ncompile:\n llm_cmd: "cat /dev/null"\n two_phase: "false"\n', + encoding="utf-8", + ) + cfg = compile_mod.load_config(store) + assert cfg.two_phase is False + + def test_parse_topics_reads_strings_and_objects() -> None: assert compile_mod.parse_topics('["Alpha", "Beta"]') == ["Alpha", "Beta"] assert compile_mod.parse_topics('[{"title": "Gamma"}]') == ["Gamma"] diff --git a/tests/test_config_coerce.py b/tests/test_config_coerce.py new file mode 100644 index 00000000..abb92a04 --- /dev/null +++ b/tests/test_config_coerce.py @@ -0,0 +1,40 @@ +"""Tests for the shared config-boolean coercer (used by compile/session_split/ +admission/inbox/recall/capture/volunteer_context/proposals config loaders).""" + +from __future__ import annotations + +import pytest + +from vouch.config_coerce import coerce_bool + + +class TestCoerceBool: + def test_real_true_passes_through(self): + assert coerce_bool(True, False) is True + + def test_real_false_passes_through(self): + assert coerce_bool(False, True) is False + + @pytest.mark.parametrize("value", ["true", "True", "TRUE", "yes", "on", "1"]) + def test_recognized_true_strings(self, value): + assert coerce_bool(value, False) is True + + @pytest.mark.parametrize("value", ["false", "False", "FALSE", "no", "off", "0"]) + def test_recognized_false_strings(self, value): + """Regression: bool("false") is True in plain Python -- the exact bug + this module exists to close off for every config loader that uses it.""" + assert coerce_bool(value, True) is False + + def test_whitespace_is_stripped(self): + assert coerce_bool(" false ", True) is False + assert coerce_bool(" true ", False) is True + + def test_unrecognized_string_falls_back_to_default(self): + assert coerce_bool("maybe", True) is True + assert coerce_bool("maybe", False) is False + + def test_non_bool_non_string_falls_back_to_default(self): + assert coerce_bool(None, True) is True + assert coerce_bool(1, False) is False # int, not bool -- not the same type + assert coerce_bool([], True) is True + assert coerce_bool({}, False) is False diff --git a/tests/test_inbox.py b/tests/test_inbox.py index d35e4902..05aa1bd7 100644 --- a/tests/test_inbox.py +++ b/tests/test_inbox.py @@ -82,6 +82,16 @@ def test_scan_disabled_via_config_is_noop(store: KBStore) -> None: assert store.list_proposals(ProposalStatus.PENDING) == [] +def test_load_config_quoted_false_does_not_enable(store: KBStore) -> None: + """Regression: bool("false") is True in plain Python, so a mistakenly + quoted `enabled: "false"` previously silently kept inbox scanning on.""" + store.config_path.write_text( + store.config_path.read_text(encoding="utf-8") + '\ninbox:\n enabled: "false"\n', + encoding="utf-8", + ) + assert inbox.load_config(store).enabled is False + + def test_watch_is_bounded_by_iterations(store: KBStore) -> None: _drop(store, "notes.md") results: list[inbox.ScanResult] = [] diff --git a/tests/test_recall.py b/tests/test_recall.py index a08a02ce..22423fb5 100644 --- a/tests/test_recall.py +++ b/tests/test_recall.py @@ -74,6 +74,13 @@ def test_load_config_override(store: KBStore) -> None: assert cfg.max_chars == 500 +def test_load_config_quoted_false_does_not_enable(store: KBStore) -> None: + """Regression: bool("false") is True in plain Python, so a mistakenly + quoted `enabled: "false"` previously silently kept recall enabled.""" + store.config_path.write_text('recall:\n enabled: "false"\n', encoding="utf-8") + assert recall.load_config(store).enabled is False + + def test_starter_config_has_recall_namespace() -> None: assert _starter_config()["recall"]["enabled"] is True diff --git a/tests/test_receipt_auto_approve.py b/tests/test_receipt_auto_approve.py index 7d972016..f3b67463 100644 --- a/tests/test_receipt_auto_approve.py +++ b/tests/test_receipt_auto_approve.py @@ -180,3 +180,52 @@ def test_auto_approve_receipts_leaves_id_conflict_pending(store: KBStore) -> Non assert other is not None assert auto_approve_receipts(store) == [] assert store.get_proposal(other.id).status is ProposalStatus.PENDING + + +def test_receipt_claim_blocked_when_gate_quoted_false_string(store: KBStore) -> None: + """Regression: bool("false") is True in plain Python. Before + _review_config() normalized this field, a mistakenly-quoted + `auto_approve_on_receipt: "false"` in config.yaml was read as *enabled* + by the raw truthy checks in resolve_pending_receipt_claim/approve, + silently granting self-approval an operator explicitly meant to disable.""" + store.config_path.write_text( + "review:\n auto_approve_on_receipt: \"false\"\n", encoding="utf-8" + ) + src = store.put_source(b"the sky is blue") + res = propose_quoted_claim( + store, text="the sky is blue", source_id=src.id, + quote="the sky is blue", proposed_by="agent-a", + ) + assert res is not None + with pytest.raises(ProposalError, match="forbidden_self_approval"): + approve(store, res.id, approved_by="agent-a") + + +def test_auto_approve_receipts_noop_when_gate_quoted_false_string(store: KBStore) -> None: + """Same regression as above, exercised through the batch drain path.""" + store.config_path.write_text( + "review:\n auto_approve_on_receipt: \"false\"\n", encoding="utf-8" + ) + src = store.put_source(b"alpha beta") + propose_quoted_claim( + store, text="mentions beta", source_id=src.id, quote="beta", + proposed_by="agent-a", + ) + assert auto_approve_receipts(store) == [] + + +def test_receipt_verified_claim_self_approves_when_gate_quoted_true_string( + store: KBStore, +) -> None: + """The quoted-string fix must not break the legitimate quoted-"true" case.""" + store.config_path.write_text( + "review:\n auto_approve_on_receipt: \"true\"\n", encoding="utf-8" + ) + src = store.put_source(b"the sky is blue today") + res = propose_quoted_claim( + store, text="the sky is blue", source_id=src.id, + quote="the sky is blue", proposed_by="agent-a", + ) + assert res is not None + claim = approve(store, res.id, approved_by="agent-a") + assert store.get_claim(claim.id).text == "the sky is blue" diff --git a/tests/test_session_split.py b/tests/test_session_split.py index ea4ff84e..b6ddfa27 100644 --- a/tests/test_session_split.py +++ b/tests/test_session_split.py @@ -49,6 +49,15 @@ def test_split_config_typo_coerces_to_default(store: KBStore) -> None: assert load_split_config(store).max_pages == 6 +def test_split_config_quoted_false_enabled_does_not_enable(store: KBStore) -> None: + """Regression: bool("false") is True in plain Python, so a mistakenly + quoted `enabled: "false"` previously silently kept splitting enabled.""" + store.config_path.write_text( + 'capture:\n split:\n enabled: "false"\n', encoding="utf-8" + ) + assert load_split_config(store).enabled is False + + def _observe(store: KBStore, sid: str, n: int, tool: str = "Edit") -> None: from vouch import capture for i in range(n): diff --git a/tests/test_volunteer_context.py b/tests/test_volunteer_context.py index 2529a162..0b970274 100644 --- a/tests/test_volunteer_context.py +++ b/tests/test_volunteer_context.py @@ -24,6 +24,13 @@ def store(tmp_path: Path) -> KBStore: return s +def test_load_config_quoted_false_does_not_enable(store: KBStore) -> None: + """Regression: bool("false") is True in plain Python, so a mistakenly + quoted `enabled: "false"` previously silently kept volunteer_context on.""" + store.config_path.write_text('volunteer:\n enabled: "false"\n') + assert volunteer_context.load_config(store).enabled is False + + def test_jwt_claim_volunteered_on_session_start(store: KBStore, monkeypatch) -> None: src = store.put_source(b"jwt spec") store.put_claim(Claim(