From 0f59323f02ade2d50761f689ae5648044c3ccd7d Mon Sep 17 00:00:00 2001 From: minion1227 Date: Thu, 30 Jul 2026 11:34:16 -0700 Subject: [PATCH 1/2] =?UTF-8?q?feat(retrieval):=20explicit=20pins=20?= =?UTF-8?q?=E2=80=94=20a=20working=20set=20that=20always=20enters=20the=20?= =?UTF-8?q?pack?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit when you are deep in one task there are three or four artifacts that should be in front of the agent every turn — the spec, the decision that constrains the design, the claim saying why the obvious approach was already rejected. whether they appear is currently up to the ranker, and a ranker optimising for the query drops them the moment the conversation moves. hot_memory and salience are the implicit version of this and decay exactly when a long task needs them not to. pinned claims and pages now lead the pack. they are capped at retrieval.pins.budget_share (default 0.3) so pins can never starve retrieval, and de-duplicated against what retrieval already found so a pinned artifact that also ranked does not take two slots. pin order is the user's stated priority, so the tail is dropped at the budget rather than silently reordered. a pin is not a gate bypass. it points at an artifact that is already approved, asserts nothing new, and creates nothing durable — there is nothing for a reviewer to review, and an unapproved artifact cannot be pinned because it does not resolve. a pin is also not a permission. lifecycle and viewer scope are re-checked on every build: a pinned claim that is later superseded, archived or redacted stops being injected, as does a pinned page that is archived or one the scope filter hides. the pin records what to prefer, never a right to see it. this is the bug class that has been fixed a dozen times on other read surfaces, so it is pinned by tests here from the start rather than after. shared pins live in committed .vouch/pins.yaml so a team shares one working set and changes show up in review; --local keeps a personal set in gitignored .vouch/pins.local.yaml, using the same .gitignore backfill retrieval telemetry uses. --expires drops a pin automatically, applied on read rather than by rewriting the file, because building a context pack must not write. no kb.* method: the issue's own open question proposes that pinning is a human act and agents may only suggest, so there is nothing to register on the agent surfaces. Closes #615 --- CHANGELOG.md | 14 ++ src/vouch/cli.py | 69 +++++++ src/vouch/context.py | 12 ++ src/vouch/pins.py | 353 ++++++++++++++++++++++++++++++++++ tests/test_pins.py | 448 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 896 insertions(+) create mode 100644 src/vouch/pins.py create mode 100644 tests/test_pins.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 886ab728..0075ceed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,20 @@ All notable changes to vouch are documented here. Format follows ## [Unreleased] ### Added +- **explicit pins — a working set that always enters the pack** (#615): + `vouch pin ` / `vouch pins list` / `vouch unpin `. Pinned claims and + pages lead every context pack instead of having to win the query each turn, + which is what `hot_memory` and `salience` cannot do — they are recency-driven + and decay exactly when a long task needs them not to. Capped at + `retrieval.pins.budget_share` (default 0.3) so pins can never starve + retrieval, and de-duplicated against what retrieval already found. Pins are + **not a gate bypass and not a permission**: they point at already-approved + artifacts, and lifecycle and viewer scope are re-checked on every build, so a + pinned claim that is later superseded/archived/redacted — or one the scope + filter hides — stops being injected. Shared pins live in committed + `.vouch/pins.yaml`; `--local` keeps a personal set in gitignored + `.vouch/pins.local.yaml`. `--expires` drops a pin automatically, applied on + read so building a pack never writes. - **`kb.explain_ranking` — why a result ranked where it did** (#432): a read-only breakdown of the retrieval pipeline. Per candidate it reports the lexical (FTS5) rank, the semantic rank, the RRF contribution, a row for every diff --git a/src/vouch/cli.py b/src/vouch/cli.py index 0801242b..160c37a6 100644 --- a/src/vouch/cli.py +++ b/src/vouch/cli.py @@ -42,6 +42,7 @@ from . import metrics as metrics_mod from . import migrations as migrations_mod from . import notify as notify_mod +from . import pins as pins_mod from . import pr_cache as prc_mod from . import provenance as prov_mod from . import recall as recall_mod @@ -115,6 +116,7 @@ def _cli_errors() -> Iterator[None]: migrations_mod.MigrationError, chatgpt_import_mod.ChatGPTImportError, codex_rollout_mod.CodexRolloutError, + pins_mod.PinError, ) as e: raise click.ClickException(str(e)) from e @@ -3760,6 +3762,73 @@ def graph(session: str | None, fmt: str) -> None: click.echo(text, nl=False) +@cli.command("pin") +@click.argument("artifact_id") +@click.option("--local", is_flag=True, + help="Pin only for me — kept out of git in .vouch/pins.local.yaml.") +@click.option("--expires", default=None, + help="Auto-drop the pin after this long (e.g. 7d) or at an ISO date.") +@click.option("--note", default=None, help="Why this is pinned.") +def pin_cmd(artifact_id: str, local: bool, expires: str | None, + note: str | None) -> None: + """Keep a claim or page in every context pack until unpinned.""" + store = _load_store() + expires_at = None + with _cli_errors(): + if expires is not None: + # parse_since counts backwards; a pin expires forwards, so mirror + # the delta around now rather than inventing a second date parser. + past = metrics_mod.parse_since(expires) + if past is not None: + expires_at = datetime.now(UTC) + (datetime.now(UTC) - past) + p = pins_mod.add_pin( + store, artifact_id, pinned_by=_whoami(), local=local, + expires_at=expires_at, note=note, + ) + where = "local" if local else "shared" + click.echo(f"pinned {p.kind}/{p.artifact_id} ({where})") + + +@cli.command("unpin") +@click.argument("artifact_id") +@click.option("--local", is_flag=True, help="Remove from the local pin set.") +def unpin_cmd(artifact_id: str, local: bool) -> None: + """Stop pinning an artifact.""" + store = _load_store() + with _cli_errors(): + removed = pins_mod.remove_pin(store, artifact_id, local=local) + if not removed: + raise click.ClickException( + f"{artifact_id} is not in the {'local' if local else 'shared'} pin set" + ) + click.echo(f"unpinned {artifact_id}") + + +@cli.group(name="pins") +def pins_group() -> None: + """The working set that always enters the context pack.""" + + +@pins_group.command("list") +@click.option("--json", "as_json", is_flag=True, help="Emit pins as JSON.") +def pins_list(as_json: bool) -> None: + """Show every live pin, shared then local.""" + store = _load_store() + with _cli_errors(): + pins = pins_mod.load_pins(store) + if as_json: + _emit_json({"pins": [p.to_dict() | {"local": p.local} for p in pins]}) + return + if not pins: + click.echo("no pins. `vouch pin ` keeps an artifact in every pack.") + return + for p in pins: + scope = "local " if p.local else "shared" + expiry = f" expires {p.expires_at:%Y-%m-%d}" if p.expires_at else "" + note = f" — {p.note}" if p.note else "" + click.echo(f"{scope} {p.kind}/{p.artifact_id}{expiry}{note}") + + @cli.group() def provenance() -> None: """Provenance graph cache operations.""" diff --git a/src/vouch/context.py b/src/vouch/context.py index 12c44912..990c178b 100644 --- a/src/vouch/context.py +++ b/src/vouch/context.py @@ -20,6 +20,7 @@ import yaml from . import graph, hot_memory, index_db, retrieval_events +from . import pins as pins_mod from . import strategy as strategy_mod from .config_coerce import coerce_bool from .embeddings.fusion import rrf_fuse @@ -830,6 +831,17 @@ def build_context_pack( items = _dedupe_near_duplicates(items) + # Pins go in front of everything retrieval chose (#615): the working set is + # a standing instruction, so it must not have to win the query every turn. + # `pinned_items` re-checks lifecycle and viewer scope on every build, so a + # pin can reorder the pack but never widen what it may contain. The budget + # share caps them, and de-duplication keeps a pinned artifact that also + # ranked from occupying two slots. + pinned = pins_mod.pinned_items(store, viewer=viewer, max_chars=max_chars) + if pinned: + pinned_keys = {(p.type, p.id) for p in pinned} + items = pinned + [i for i in items if (i.type, i.id) not in pinned_keys] + failed: list[str] = [] uncited: list[str] = [] budget_truncated = False diff --git a/src/vouch/pins.py b/src/vouch/pins.py new file mode 100644 index 00000000..8dc39b1c --- /dev/null +++ b/src/vouch/pins.py @@ -0,0 +1,353 @@ +"""Explicit pins — a working set that always enters the context pack (#615). + +Retrieval ranks for the *query*, so the three or four artifacts that should be +in front of an agent on every turn of a long task drop out the moment the +conversation moves. ``hot_memory`` and ``salience`` are the implicit, +recency-driven version of this and decay exactly when a long task needs them +not to. A pin is the explicit version: this specific artifact, until I say +otherwise. + +**Not a gate bypass.** A pin is a pointer to an artifact that is *already* +approved — it asserts nothing new and creates no durable knowledge, so there is +nothing for a reviewer to review. Pinning cannot introduce a claim, and an +artifact that was never approved cannot be pinned in the first place. + +**Pins never resurrect retired knowledge.** A pinned claim that is later +superseded, archived or redacted, or a pinned page that is archived, stops +being injected — the pin does not override lifecycle. The same applies to +viewer scope: a pin is not a way to see something the scope filter hides. +Pins are a ranking instruction, never a permission. + +**Pins cannot starve retrieval.** Injected pins are capped at +``retrieval.pins.budget_share`` of the pack's character budget (default 0.3), +so the ranker always keeps the majority of the pack. + +Two files, both under ``.vouch/``: + +* ``pins.yaml`` — committed, so a team shares one working set and changes to + it show up in review like everything else. +* ``pins.local.yaml`` — gitignored, for a personal working set. +""" + +from __future__ import annotations + +import logging +from dataclasses import dataclass +from datetime import UTC, datetime +from pathlib import Path +from typing import TYPE_CHECKING, Any + +import yaml + +from .config_coerce import coerce_bool + +if TYPE_CHECKING: # pragma: no cover - typing only + from .storage import KBStore + +logger = logging.getLogger(__name__) + +SHARED_FILENAME = "pins.yaml" +LOCAL_FILENAME = "pins.local.yaml" + +DEFAULT_ENABLED = True +DEFAULT_BUDGET_SHARE = 0.3 +DEFAULT_MAX_CHARS = 2000 + +# Kinds a pin may point at. Claims and pages are what a working set is made of; +# the rest of the artifact kinds are graph structure, not reading material. +PINNABLE_KINDS = ("claim", "page") + + +class PinError(RuntimeError): + """A pin could not be created or removed.""" + + +@dataclass(frozen=True) +class Pin: + """One pinned artifact.""" + + artifact_id: str + kind: str + pinned_at: datetime + pinned_by: str + expires_at: datetime | None = None + note: str | None = None + local: bool = False + + def expired(self, *, now: datetime | None = None) -> bool: + if self.expires_at is None: + return False + return (now or datetime.now(UTC)) >= self.expires_at + + def to_dict(self) -> dict[str, Any]: + out: dict[str, Any] = { + "id": self.artifact_id, + "kind": self.kind, + "pinned_at": self.pinned_at.isoformat(timespec="seconds"), + "pinned_by": self.pinned_by, + } + if self.expires_at is not None: + out["expires_at"] = self.expires_at.isoformat(timespec="seconds") + if self.note: + out["note"] = self.note + return out + + +@dataclass(frozen=True) +class PinsConfig: + enabled: bool = DEFAULT_ENABLED + budget_share: float = DEFAULT_BUDGET_SHARE + + +def load_config(store: KBStore) -> PinsConfig: + """Read ``retrieval.pins`` from config.yaml; fall back to defaults.""" + try: + loaded = yaml.safe_load(store.config_path.read_text(encoding="utf-8")) + except (OSError, yaml.YAMLError): + return PinsConfig() + if not isinstance(loaded, dict): + return PinsConfig() + retrieval = loaded.get("retrieval") + raw = retrieval.get("pins") if isinstance(retrieval, dict) else None + if not isinstance(raw, dict): + return PinsConfig() + share = raw.get("budget_share", DEFAULT_BUDGET_SHARE) + if not isinstance(share, int | float) or isinstance(share, bool): + share = DEFAULT_BUDGET_SHARE + # A share outside (0, 1] is a config typo, not an instruction to disable + # retrieval or to let pins take the whole pack. + share = min(1.0, max(0.0, float(share))) + return PinsConfig( + enabled=coerce_bool(raw.get("enabled", DEFAULT_ENABLED), DEFAULT_ENABLED), + budget_share=share, + ) + + +def _path(store: KBStore, *, local: bool) -> Path: + return store.kb_dir / (LOCAL_FILENAME if local else SHARED_FILENAME) + + +def _ensure_ignored(store: KBStore) -> None: + """Keep the local pin set out of git, the way retrieval telemetry is. + + Best-effort: an unwritable .gitignore must not stop someone pinning. + """ + gi = store.kb_dir / ".gitignore" + try: + text = gi.read_text(encoding="utf-8") if gi.exists() else "" + if LOCAL_FILENAME in text: + return + if text and not text.endswith("\n"): + text += "\n" + gi.write_text(f"{text}{LOCAL_FILENAME}\n", encoding="utf-8") + except OSError as e: # pragma: no cover - filesystem edge + logger.debug("pins: could not update .gitignore (%s)", e) + + +def _parse_dt(value: Any) -> datetime | None: + if not isinstance(value, str): + return None + try: + parsed = datetime.fromisoformat(value) + except ValueError: + return None + return parsed.replace(tzinfo=UTC) if parsed.tzinfo is None else parsed + + +def _read_file(store: KBStore, *, local: bool) -> list[Pin]: + """Parse one pin file. A malformed entry is skipped, never fatal.""" + path = _path(store, local=local) + if not path.exists(): + return [] + try: + loaded = yaml.safe_load(path.read_text(encoding="utf-8")) + except (OSError, yaml.YAMLError): + return [] + rows = loaded.get("pins") if isinstance(loaded, dict) else loaded + if not isinstance(rows, list): + return [] + out: list[Pin] = [] + for row in rows: + if not isinstance(row, dict): + continue + artifact_id = str(row.get("id", "")).strip() + kind = str(row.get("kind", "")).strip() + if not artifact_id or kind not in PINNABLE_KINDS: + continue + out.append(Pin( + artifact_id=artifact_id, + kind=kind, + pinned_at=_parse_dt(row.get("pinned_at")) or datetime.now(UTC), + pinned_by=str(row.get("pinned_by", "unknown")), + expires_at=_parse_dt(row.get("expires_at")), + note=row.get("note") if isinstance(row.get("note"), str) else None, + local=local, + )) + return out + + +def _write_file(store: KBStore, pins: list[Pin], *, local: bool) -> None: + if local: + _ensure_ignored(store) + path = _path(store, local=local) + if not pins: + path.unlink(missing_ok=True) + return + path.write_text( + yaml.safe_dump({"pins": [p.to_dict() for p in pins]}, sort_keys=False), + encoding="utf-8", + ) + + +def load_pins( + store: KBStore, *, include_local: bool = True, now: datetime | None = None +) -> list[Pin]: + """Every live pin, shared first then local, expired ones dropped. + + Expiry is applied on read rather than by rewriting the file: reading the + KB must not mutate it, and a pin set that quietly rewrote itself during a + context build would be a write on the read path. + """ + pins = _read_file(store, local=False) + if include_local: + pins += _read_file(store, local=True) + seen: set[tuple[str, str]] = set() + live: list[Pin] = [] + for pin in pins: + key = (pin.kind, pin.artifact_id) + if key in seen or pin.expired(now=now): + continue + seen.add(key) + live.append(pin) + return live + + +def _resolve_kind(store: KBStore, artifact_id: str) -> str | None: + """``"claim"`` / ``"page"`` / ``None`` — what this id actually is.""" + from .storage import ArtifactNotFoundError + + try: + store.get_claim(artifact_id) + return "claim" + except ArtifactNotFoundError: + pass + try: + store.get_page(artifact_id) + return "page" + except ArtifactNotFoundError: + return None + + +def add_pin( + store: KBStore, + artifact_id: str, + *, + pinned_by: str, + local: bool = False, + expires_at: datetime | None = None, + note: str | None = None, +) -> Pin: + """Pin an existing claim or page. Raises :class:`PinError` otherwise.""" + artifact_id = artifact_id.strip() + if not artifact_id: + raise PinError("pin needs an artifact id") + kind = _resolve_kind(store, artifact_id) + if kind is None: + raise PinError( + f"unknown artifact {artifact_id!r}: a pin points at an approved " + f"claim or page, so there is nothing to pin until it exists" + ) + existing = _read_file(store, local=local) + kept = [p for p in existing if p.artifact_id != artifact_id] + pin = Pin( + artifact_id=artifact_id, + kind=kind, + pinned_at=datetime.now(UTC), + pinned_by=pinned_by, + expires_at=expires_at, + note=note, + local=local, + ) + _write_file(store, [*kept, pin], local=local) + return pin + + +def remove_pin(store: KBStore, artifact_id: str, *, local: bool = False) -> bool: + """Unpin. Returns False when it was not pinned in that set.""" + artifact_id = artifact_id.strip() + existing = _read_file(store, local=local) + kept = [p for p in existing if p.artifact_id != artifact_id] + if len(kept) == len(existing): + return False + _write_file(store, kept, local=local) + return True + + +def pinned_items( + store: KBStore, + *, + viewer: Any = None, + max_chars: int | None = None, + config: PinsConfig | None = None, +) -> list[Any]: + """Live pinned artifacts as ``ContextItem``s, within the pin budget. + + Lifecycle and scope are re-checked on every build: a pin records *what* to + prefer, never a right to see it. Retired artifacts and out-of-scope ones + are dropped exactly as retrieval would drop them. + """ + from .context import _RETRACTED_CLAIM_STATUSES, _page_is_live + from .models import ContextItem + from .scoping import artifact_scope_for_hit, is_visible + from .storage import ArtifactNotFoundError + + cfg = config or load_config(store) + if not cfg.enabled: + return [] + + budget = int((max_chars or DEFAULT_MAX_CHARS) * cfg.budget_share) + items: list[Any] = [] + used = 0 + for pin in load_pins(store): + summary: str | None = None + citations: list[str] = [] + if pin.kind == "claim": + try: + claim = store.get_claim(pin.artifact_id) + except ArtifactNotFoundError: + continue + if claim.status in _RETRACTED_CLAIM_STATUSES: + continue + summary = claim.text + citations = list(claim.evidence) + else: + # No missing-page guard: `_page_is_live` returns False for a page + # whose yaml is gone as well as for an archived one, so anything + # reaching the read here has already been proved readable. + if not _page_is_live(store, pin.artifact_id): + continue + summary = store.get_page(pin.artifact_id).title + + if viewer is not None: + scope = artifact_scope_for_hit(store, pin.kind, pin.artifact_id) + if scope is not None and not is_visible(scope, viewer): + continue + + summary = summary or "" + # Stop at the first pin that would breach the share rather than + # skipping it and taking a later, smaller one — pin order is the + # user's stated priority and silently reordering it would be worse + # than dropping the tail. + if used + len(summary) > budget and items: + break + used += len(summary) + items.append(ContextItem( + id=pin.artifact_id, + type=pin.kind, # type: ignore[arg-type] + summary=summary, + score=1.0, + backend="pin", + citations=citations, + freshness="unknown", + )) + return items diff --git a/tests/test_pins.py b/tests/test_pins.py new file mode 100644 index 00000000..910bc4f1 --- /dev/null +++ b/tests/test_pins.py @@ -0,0 +1,448 @@ +"""Explicit pins — issue #615.""" + +from __future__ import annotations + +import json +from datetime import UTC, datetime, timedelta +from pathlib import Path + +import pytest +import yaml +from click.testing import CliRunner + +from vouch import health, lifecycle, pins, proposals +from vouch.cli import cli +from vouch.context import build_context_pack +from vouch.models import ArtifactScope, Page, PageStatus, PageType, Visibility +from vouch.storage import KBStore + + +@pytest.fixture +def store(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> KBStore: + kb = KBStore.init(tmp_path) + monkeypatch.chdir(kb.root) + return kb + + +def _claim(store: KBStore, text: str) -> str: + src = store.put_source(text.encode("utf-8") + b" source bytes") + pr = proposals.propose_claim( + store, text=text, evidence=[src.id], proposed_by="agent" + ) + return proposals.approve(store, pr.id, approved_by="reviewer").id + + +def _ids(pack: dict) -> list[str]: + return [i["id"] for i in pack["items"]] + + +def _write_pins_cfg(store: KBStore, **pins_cfg: object) -> None: + store.config_path.write_text( + yaml.safe_dump({"retrieval": {"pins": pins_cfg}}), encoding="utf-8" + ) + + +# --- the core behaviour --------------------------------------------------- + + +def test_pinned_artifact_leads_a_pack_it_would_not_have_entered( + store: KBStore +) -> None: + """The whole point: a pin does not have to win the query.""" + spec = _claim(store, "the spec says tokens rotate every 24 hours") + other = _claim(store, "kubernetes ingress uses nginx") + health.rebuild_index(store) + + before = build_context_pack(store, query="kubernetes", limit=5, max_chars=2000) + assert spec not in _ids(before) + + pins.add_pin(store, spec, pinned_by="human") + after = build_context_pack(store, query="kubernetes", limit=5, max_chars=2000) + assert _ids(after)[0] == spec + assert other in _ids(after) + + +def test_pinned_item_is_marked_with_the_pin_backend(store: KBStore) -> None: + spec = _claim(store, "the spec says tokens rotate every 24 hours") + health.rebuild_index(store) + pins.add_pin(store, spec, pinned_by="human") + + pack = build_context_pack(store, query="anything", limit=5, max_chars=2000) + assert pack["items"][0]["backend"] == "pin" + + +def test_a_pinned_artifact_that_also_ranks_appears_once(store: KBStore) -> None: + spec = _claim(store, "the spec says tokens rotate every 24 hours") + health.rebuild_index(store) + pins.add_pin(store, spec, pinned_by="human") + + ids = _ids(build_context_pack(store, query="tokens", limit=5, max_chars=2000)) + assert ids.count(spec) == 1 + + +def test_pin_order_is_preserved(store: KBStore) -> None: + first = _claim(store, "the spec says tokens rotate every 24 hours") + second = _claim(store, "we rejected polling because it doubles cost") + health.rebuild_index(store) + pins.add_pin(store, first, pinned_by="human") + pins.add_pin(store, second, pinned_by="human") + + assert _ids(build_context_pack( + store, query="unrelated", limit=5, max_chars=2000 + ))[:2] == [first, second] + + +# --- a pin is not a permission ------------------------------------------- + + +@pytest.mark.parametrize("retire", ["archive", "supersede"]) +def test_a_retired_claim_stops_being_injected(store: KBStore, retire: str) -> None: + """Lifecycle beats a pin — a pin records what to prefer, not a right.""" + spec = _claim(store, "the spec says tokens rotate every 24 hours") + health.rebuild_index(store) + pins.add_pin(store, spec, pinned_by="human") + assert spec in _ids(build_context_pack(store, query="x", limit=5, max_chars=2000)) + + if retire == "archive": + lifecycle.archive(store, claim_id=spec, actor="reviewer") + else: + newer = _claim(store, "the spec now says tokens rotate every 12 hours") + lifecycle.supersede( + store, old_claim_id=spec, new_claim_id=newer, actor="reviewer" + ) + + assert spec not in _ids(build_context_pack( + store, query="x", limit=5, max_chars=2000 + )) + + +def test_an_archived_page_stops_being_injected(store: KBStore) -> None: + store.put_page(Page(id="p-live", title="design notes", body="b", + type=PageType.CONCEPT)) + health.rebuild_index(store) + pins.add_pin(store, "p-live", pinned_by="human") + assert "p-live" in _ids(build_context_pack( + store, query="x", limit=5, max_chars=2000 + )) + + page = store.get_page("p-live") + page.status = PageStatus.ARCHIVED + store.update_page(page) + + assert "p-live" not in _ids(build_context_pack( + store, query="x", limit=5, max_chars=2000 + )) + + +def test_a_pin_cannot_widen_viewer_scope(store: KBStore) -> None: + """Pinning must not be a way to see what the scope filter hides.""" + src = store.put_source(b"private source bytes") + pr = proposals.propose_claim( + store, text="the private staging key rotates weekly", + evidence=[src.id], proposed_by="agent", + scope=ArtifactScope(visibility=Visibility.PRIVATE, project="other-project"), + ) + private = proposals.approve(store, pr.id, approved_by="reviewer").id + health.rebuild_index(store) + pins.add_pin(store, private, pinned_by="human") + + pack = build_context_pack( + store, query="x", limit=5, max_chars=2000, project="this-project" + ) + assert private not in _ids(pack) + + +def test_pinning_an_unknown_artifact_is_refused(store: KBStore) -> None: + with pytest.raises(pins.PinError, match="unknown artifact"): + pins.add_pin(store, "never-existed", pinned_by="human") + + +def test_pinning_an_empty_id_is_refused(store: KBStore) -> None: + with pytest.raises(pins.PinError, match="needs an artifact id"): + pins.add_pin(store, " ", pinned_by="human") + + +# --- budget share --------------------------------------------------------- + + +def test_pins_cannot_starve_retrieval(store: KBStore) -> None: + """The share caps pins even when many are set.""" + pinned = [_claim(store, f"pinned working-set item number {i} " + "x" * 80) + for i in range(6)] + health.rebuild_index(store) + for cid in pinned: + pins.add_pin(store, cid, pinned_by="human") + + _write_pins_cfg(store, budget_share=0.3) + items = pins.pinned_items(store, max_chars=1000) + assert 0 < len(items) < len(pinned) + assert sum(len(i.summary) for i in items) <= 1000 * 0.3 + max( + len(i.summary) for i in items + ) + + +def test_at_least_one_pin_survives_a_tiny_budget(store: KBStore) -> None: + """The first pin is never dropped — a share of ~0 still honours one.""" + spec = _claim(store, "the spec says tokens rotate every 24 hours") + health.rebuild_index(store) + pins.add_pin(store, spec, pinned_by="human") + + _write_pins_cfg(store, budget_share=0.01) + assert [i.id for i in pins.pinned_items(store, max_chars=100)] == [spec] + + +def test_pins_can_be_disabled_in_config(store: KBStore) -> None: + spec = _claim(store, "the spec says tokens rotate every 24 hours") + health.rebuild_index(store) + pins.add_pin(store, spec, pinned_by="human") + + _write_pins_cfg(store, enabled=False) + assert pins.pinned_items(store, max_chars=2000) == [] + assert spec not in _ids(build_context_pack( + store, query="x", limit=5, max_chars=2000 + )) + + +@pytest.mark.parametrize( + ("raw", "expected"), + [(0.5, 0.5), (2.0, 1.0), (-1.0, 0.0), ("nonsense", pins.DEFAULT_BUDGET_SHARE), + (True, pins.DEFAULT_BUDGET_SHARE)], +) +def test_budget_share_is_read_defensively( + store: KBStore, raw: object, expected: float +) -> None: + _write_pins_cfg(store, budget_share=raw) + assert pins.load_config(store).budget_share == expected + + +def test_missing_config_falls_back_to_defaults(store: KBStore) -> None: + cfg = pins.load_config(store) + assert cfg.enabled is pins.DEFAULT_ENABLED + assert cfg.budget_share == pins.DEFAULT_BUDGET_SHARE + + +# --- storage, expiry, local vs shared ------------------------------------- + + +def test_expired_pins_are_dropped_on_read_without_rewriting(store: KBStore) -> None: + """Reading the KB must not mutate it.""" + spec = _claim(store, "the spec says tokens rotate every 24 hours") + pins.add_pin( + store, spec, pinned_by="human", + expires_at=datetime.now(UTC) - timedelta(days=1), + ) + path = store.kb_dir / pins.SHARED_FILENAME + before = path.read_text(encoding="utf-8") + + assert pins.load_pins(store) == [] + assert path.read_text(encoding="utf-8") == before + + +def test_a_future_expiry_stays_live(store: KBStore) -> None: + spec = _claim(store, "the spec says tokens rotate every 24 hours") + pins.add_pin( + store, spec, pinned_by="human", + expires_at=datetime.now(UTC) + timedelta(days=7), + ) + assert [p.artifact_id for p in pins.load_pins(store)] == [spec] + + +def test_local_pins_are_gitignored_and_excludable(store: KBStore) -> None: + spec = _claim(store, "the spec says tokens rotate every 24 hours") + pins.add_pin(store, spec, pinned_by="human", local=True) + + assert pins.LOCAL_FILENAME in (store.kb_dir / ".gitignore").read_text() + assert [p.artifact_id for p in pins.load_pins(store)] == [spec] + assert pins.load_pins(store, include_local=False) == [] + + +def test_shared_wins_when_an_artifact_is_pinned_in_both_sets( + store: KBStore +) -> None: + spec = _claim(store, "the spec says tokens rotate every 24 hours") + pins.add_pin(store, spec, pinned_by="human", note="shared") + pins.add_pin(store, spec, pinned_by="human", local=True, note="local") + + live = pins.load_pins(store) + assert len(live) == 1 + assert live[0].local is False + assert live[0].note == "shared" + + +def test_repinning_replaces_rather_than_duplicates(store: KBStore) -> None: + spec = _claim(store, "the spec says tokens rotate every 24 hours") + pins.add_pin(store, spec, pinned_by="human", note="first") + pins.add_pin(store, spec, pinned_by="human", note="second") + + live = pins.load_pins(store) + assert len(live) == 1 + assert live[0].note == "second" + + +def test_removing_the_last_pin_removes_the_file(store: KBStore) -> None: + spec = _claim(store, "the spec says tokens rotate every 24 hours") + pins.add_pin(store, spec, pinned_by="human") + assert pins.remove_pin(store, spec) is True + assert not (store.kb_dir / pins.SHARED_FILENAME).exists() + assert pins.remove_pin(store, spec) is False + + +def test_malformed_pin_rows_are_skipped_not_fatal(store: KBStore) -> None: + spec = _claim(store, "the spec says tokens rotate every 24 hours") + (store.kb_dir / pins.SHARED_FILENAME).write_text( + yaml.safe_dump({"pins": [ + "not-a-mapping", + {"kind": "claim"}, # no id + {"id": "x"}, # no kind + {"id": "y", "kind": "entity"}, # unpinnable kind + {"id": spec, "kind": "claim", "expires_at": "not-a-date"}, + ]}), + encoding="utf-8", + ) + live = pins.load_pins(store) + assert [p.artifact_id for p in live] == [spec] + assert live[0].expires_at is None + + +def test_unreadable_pin_file_is_not_fatal(store: KBStore) -> None: + (store.kb_dir / pins.SHARED_FILENAME).write_text("{{ not yaml", encoding="utf-8") + assert pins.load_pins(store) == [] + + +def test_a_page_pin_resolves_to_its_title(store: KBStore) -> None: + store.put_page(Page(id="p1", title="design notes", body="body", + type=PageType.CONCEPT)) + pin = pins.add_pin(store, "p1", pinned_by="human") + assert pin.kind == "page" + assert pins.pinned_items(store, max_chars=2000)[0].summary == "design notes" + + +# --- cli ------------------------------------------------------------------ + + +def test_cli_pin_list_unpin_roundtrip(store: KBStore) -> None: + spec = _claim(store, "the spec says tokens rotate every 24 hours") + runner = CliRunner() + + empty = runner.invoke(cli, ["pins", "list"]) + assert empty.exit_code == 0 + assert "no pins" in empty.output + + added = runner.invoke(cli, ["pin", spec, "--note", "the constraint"]) + assert added.exit_code == 0, added.output + assert "pinned claim/" in added.output + + listed = runner.invoke(cli, ["pins", "list"]) + assert listed.exit_code == 0 + assert spec in listed.output + assert "the constraint" in listed.output + + removed = runner.invoke(cli, ["unpin", spec]) + assert removed.exit_code == 0, removed.output + assert "no pins" in runner.invoke(cli, ["pins", "list"]).output + + +def test_cli_pin_json_output(store: KBStore) -> None: + spec = _claim(store, "the spec says tokens rotate every 24 hours") + CliRunner().invoke(cli, ["pin", spec]) + res = CliRunner().invoke(cli, ["pins", "list", "--json"]) + assert res.exit_code == 0, res.output + payload = json.loads(res.output) + assert payload["pins"][0]["id"] == spec + assert payload["pins"][0]["local"] is False + + +def test_cli_local_pin_is_labelled(store: KBStore) -> None: + spec = _claim(store, "the spec says tokens rotate every 24 hours") + res = CliRunner().invoke(cli, ["pin", spec, "--local"]) + assert res.exit_code == 0, res.output + assert "(local)" in res.output + assert "local " in CliRunner().invoke(cli, ["pins", "list"]).output + + +def test_cli_expiry_is_accepted_and_shown(store: KBStore) -> None: + spec = _claim(store, "the spec says tokens rotate every 24 hours") + assert CliRunner().invoke(cli, ["pin", spec, "--expires", "7d"]).exit_code == 0 + listed = CliRunner().invoke(cli, ["pins", "list"]) + assert "expires" in listed.output + assert pins.load_pins(store)[0].expires_at is not None + + +def test_cli_unknown_artifact_is_a_clean_error(store: KBStore) -> None: + """A domain error must render as `Error: ...`, never a traceback.""" + res = CliRunner().invoke(cli, ["pin", "never-existed"]) + assert res.exit_code != 0 + assert "Error: unknown artifact" in res.output + assert "Traceback" not in res.output + + +def test_cli_unpinning_something_unpinned_is_a_clean_error(store: KBStore) -> None: + res = CliRunner().invoke(cli, ["unpin", "never-pinned"]) + assert res.exit_code != 0 + assert "not in the shared pin set" in res.output + assert "Traceback" not in res.output + + +# --- defensive paths ------------------------------------------------------ + + +def test_unreadable_config_falls_back_to_defaults(store: KBStore) -> None: + store.config_path.write_text("{{ not yaml", encoding="utf-8") + assert pins.load_config(store).budget_share == pins.DEFAULT_BUDGET_SHARE + + +def test_non_mapping_config_falls_back_to_defaults(store: KBStore) -> None: + store.config_path.write_text("- a list, not a mapping\n", encoding="utf-8") + assert pins.load_config(store).enabled is pins.DEFAULT_ENABLED + + +def test_config_without_a_retrieval_block_falls_back(store: KBStore) -> None: + store.config_path.write_text( + yaml.safe_dump({"review": {"auto_approve_on_receipt": True}}), + encoding="utf-8", + ) + assert pins.load_config(store).budget_share == pins.DEFAULT_BUDGET_SHARE + + +def test_pin_file_that_is_not_a_list_yields_no_pins(store: KBStore) -> None: + (store.kb_dir / pins.SHARED_FILENAME).write_text( + yaml.safe_dump({"pins": {"not": "a list"}}), encoding="utf-8" + ) + assert pins.load_pins(store) == [] + + +def test_gitignore_is_only_appended_once(store: KBStore) -> None: + first = _claim(store, "the spec says tokens rotate every 24 hours") + second = _claim(store, "we rejected polling because it doubles cost") + pins.add_pin(store, first, pinned_by="human", local=True) + pins.add_pin(store, second, pinned_by="human", local=True) + + text = (store.kb_dir / ".gitignore").read_text(encoding="utf-8") + assert text.count(pins.LOCAL_FILENAME) == 1 + + +def test_gitignore_without_a_trailing_newline_is_extended_cleanly( + store: KBStore +) -> None: + gi = store.kb_dir / ".gitignore" + gi.write_text("state.db", encoding="utf-8") # no trailing newline + spec = _claim(store, "the spec says tokens rotate every 24 hours") + pins.add_pin(store, spec, pinned_by="human", local=True) + + lines = gi.read_text(encoding="utf-8").splitlines() + assert "state.db" in lines + assert pins.LOCAL_FILENAME in lines + + +def test_a_pinned_artifact_deleted_from_disk_is_skipped(store: KBStore) -> None: + """The yaml can go while the pin file survives; that must not raise.""" + spec = _claim(store, "the spec says tokens rotate every 24 hours") + store.put_page(Page(id="p1", title="design notes", body="b", + type=PageType.CONCEPT)) + pins.add_pin(store, spec, pinned_by="human") + pins.add_pin(store, "p1", pinned_by="human") + assert len(pins.pinned_items(store, max_chars=4000)) == 2 + + store._claim_path(spec).unlink() + store._page_path("p1").unlink() + assert pins.pinned_items(store, max_chars=4000) == [] From 29a4af89fba420fd40083e7fa75bb11f79bbf54b Mon Sep 17 00:00:00 2001 From: minion1227 Date: Thu, 30 Jul 2026 12:26:45 -0700 Subject: [PATCH 2/2] fix(pins): parse --expires iso dates forwards, not mirrored parse_since returns iso input unchanged and only counts durations backwards, so mirroring its result around now was right for 7d and wrong for a date: --expires 2026-08-15 computed now + (now - 2026-08-15), a timestamp a month in the past. load_pins drops expired pins on read, so the pin vanished the moment it was written, with no error. iso input is now parsed directly and only durations are mirrored. a spec resolving to no bound (all, empty) is rejected rather than silently meaning never, which is already what omitting the flag does. also de-duplicate retrieval against pins on near-duplicate text, not just exact (type, id). _dedupe_near_duplicates runs before pins are injected, so it never compares a retrieved item to a pin, and the same knowledge stored under a second id could take a second slot. the comparison runs at the injection site rather than by moving that pass below it: the pass keeps the highest-scored member of a cluster, and pages_first multiplies a page's score past a pin's flat 1.0, so moving it would let retrieval evict a pin. --- src/vouch/cli.py | 29 ++++++++++--- src/vouch/context.py | 35 +++++++++++++-- tests/test_pins.py | 101 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 157 insertions(+), 8 deletions(-) diff --git a/src/vouch/cli.py b/src/vouch/cli.py index da29f505..95348105 100644 --- a/src/vouch/cli.py +++ b/src/vouch/cli.py @@ -3776,11 +3776,30 @@ def pin_cmd(artifact_id: str, local: bool, expires: str | None, expires_at = None with _cli_errors(): if expires is not None: - # parse_since counts backwards; a pin expires forwards, so mirror - # the delta around now rather than inventing a second date parser. - past = metrics_mod.parse_since(expires) - if past is not None: - expires_at = datetime.now(UTC) + (datetime.now(UTC) - past) + # An absolute date is already the answer, so only a duration gets + # mirrored. parse_since returns ISO input unchanged, and mirroring + # that around now turns a future date into a past one — the pin + # would be created already expired, silently. + try: + expires_at = datetime.fromisoformat(expires) + except ValueError: + # Not a date, so read it as a duration counted backwards and + # mirror it forwards. + now = datetime.now(UTC) + past = metrics_mod.parse_since(expires) + if past is None: + # "all" and "" mean "no lower bound" to parse_since. As an + # expiry that would silently mean "never", which is already + # what omitting the flag does — so it is a typo, not a + # request worth honouring. + raise click.ClickException( + f"--expires {expires!r}: expected a duration like '7d' " + "or an ISO date like '2026-08-15'" + ) from None + expires_at = now + (now - past) + else: + if expires_at.tzinfo is None: + expires_at = expires_at.replace(tzinfo=UTC) p = pins_mod.add_pin( store, artifact_id, pinned_by=_whoami(), local=local, expires_at=expires_at, note=note, diff --git a/src/vouch/context.py b/src/vouch/context.py index 990c178b..49b4a4ab 100644 --- a/src/vouch/context.py +++ b/src/vouch/context.py @@ -721,6 +721,16 @@ def _jaccard(a: set[str], b: set[str]) -> float: return len(a & b) / len(a | b) +# The near-duplicate heuristic, shared by the retrieval pass and the pin +# injection below it so the two cannot drift to different notions of "same". +_NEAR_DUP_THRESHOLD = 0.85 +_NEAR_DUP_TOKENS = 40 + + +def _near_dup_tokens(summary: str) -> set[str]: + return set(summary.lower().split()[:_NEAR_DUP_TOKENS]) + + def _dedupe_near_duplicates(items: list[ContextItem]) -> list[ContextItem]: """Drop items whose summary is near-identical to a higher-scored one. @@ -737,8 +747,8 @@ def _dedupe_near_duplicates(items: list[ContextItem]) -> list[ContextItem]: kept_tokens: list[set[str]] = [] order = sorted(range(len(items)), key=lambda i: items[i].score, reverse=True) for idx in order: - toks = set(items[idx].summary.lower().split()[:40]) - if any(_jaccard(toks, seen) >= 0.85 for seen in kept_tokens): + toks = _near_dup_tokens(items[idx].summary) + if any(_jaccard(toks, seen) >= _NEAR_DUP_THRESHOLD for seen in kept_tokens): dropped.add(idx) continue kept_tokens.append(toks) @@ -839,8 +849,27 @@ def build_context_pack( # ranked from occupying two slots. pinned = pins_mod.pinned_items(store, viewer=viewer, max_chars=max_chars) if pinned: + # Exact `(type, id)` de-duplication is not enough: the same knowledge + # can be stored under a second id, and `_dedupe_near_duplicates` ran + # before the pins existed, so a retrieved near-duplicate of a pin has + # never been compared against it. + # + # Deliberately not solved by moving the pass below this injection: it + # keeps the *highest-scored* member of a cluster rather than the first, + # and `pages_first` multiplies a page's score by `boost` (1.25 by + # default), so a retrieved page can outscore a pin's flat 1.0 and evict + # it. That is precisely the outcome pinning exists to prevent, so the + # comparison runs here instead, where the pin always wins. pinned_keys = {(p.type, p.id) for p in pinned} - items = pinned + [i for i in items if (i.type, i.id) not in pinned_keys] + pinned_tokens = [_near_dup_tokens(p.summary) for p in pinned] + items = pinned + [ + i for i in items + if (i.type, i.id) not in pinned_keys + and not any( + _jaccard(_near_dup_tokens(i.summary), pt) >= _NEAR_DUP_THRESHOLD + for pt in pinned_tokens + ) + ] failed: list[str] = [] uncited: list[str] = [] diff --git a/tests/test_pins.py b/tests/test_pins.py index 910bc4f1..ef6f7879 100644 --- a/tests/test_pins.py +++ b/tests/test_pins.py @@ -71,6 +71,60 @@ def test_pinned_item_is_marked_with_the_pin_backend(store: KBStore) -> None: assert pack["items"][0]["backend"] == "pin" +def test_a_pin_evicts_its_near_duplicate_from_retrieval(store: KBStore) -> None: + """The same knowledge stored under a second id must not occupy two slots. + + `_dedupe_near_duplicates` runs before pins are injected, so it never gets + to compare a retrieved item against a pin. Exact `(type, id)` matching + does not catch it either, because the near-duplicate has a different id. + """ + # 9 of 10 shared tokens -> Jaccard 0.9, over the 0.85 threshold, while the + # slugified ids stay distinct so exact matching cannot catch it. + pinned = _claim(store, "tokens rotate every twenty four hours per the spec") + twin = _claim(store, "tokens rotate every twenty four hours per the spec doc") + health.rebuild_index(store) + pins.add_pin(store, pinned, pinned_by="human") + + ids = _ids(build_context_pack(store, query="tokens rotate", + limit=5, max_chars=2000)) + assert ids[0] == pinned + assert twin not in ids, "near-duplicate of a pin took a second slot" + + +def test_near_duplicate_eviction_keeps_the_pin_not_the_higher_score( + store: KBStore, +) -> None: + """The pin wins the collision even when retrieval scores its twin higher. + + Deliberately not fixed by moving `_dedupe_near_duplicates` below the pin + injection: that pass keeps the highest-scored member of a cluster, and + `pages_first` multiplies a page's score past a pin's flat 1.0 — which + would evict the pin, the one thing pinning must never allow. + """ + store.config_path.write_text( + yaml.safe_dump({ + "retrieval": { + "pins": {"enabled": True}, + "pages_first": {"enabled": True, "boost": 5.0}, + } + }), + encoding="utf-8", + ) + text = "tokens rotate every twenty four hours per the spec" + pinned = _claim(store, text) + store.put_page(Page( + id="p-twin", title=text, body="same knowledge, page form", + type=PageType.CONCEPT, status=PageStatus.DRAFT, + )) + health.rebuild_index(store) + pins.add_pin(store, pinned, pinned_by="human") + + ids = _ids(build_context_pack(store, query="tokens rotate", + limit=5, max_chars=2000)) + assert ids[0] == pinned + assert "p-twin" not in ids + + def test_a_pinned_artifact_that_also_ranks_appears_once(store: KBStore) -> None: spec = _claim(store, "the spec says tokens rotate every 24 hours") health.rebuild_index(store) @@ -368,6 +422,53 @@ def test_cli_expiry_is_accepted_and_shown(store: KBStore) -> None: assert pins.load_pins(store)[0].expires_at is not None +def test_cli_expiry_accepts_an_iso_date_in_the_future(store: KBStore) -> None: + """An ISO date must expire in the future, not be mirrored into the past. + + `parse_since` returns ISO input unchanged, so mirroring it around now (the + handling durations need) turned a future date into a past one and created + the pin already expired — silently, since nothing rejects it. + """ + spec = _claim(store, "the spec says tokens rotate every 24 hours") + tomorrow = (datetime.now(UTC) + timedelta(days=1)).date().isoformat() + + res = CliRunner().invoke(cli, ["pin", spec, "--expires", tomorrow]) + assert res.exit_code == 0, res.output + + # load_pins drops expired pins on read, so the old inversion made the pin + # disappear outright — this list is empty before the fix. + live = pins.load_pins(store) + assert [p.artifact_id for p in live] == [spec] + assert live[0].expires_at is not None + assert live[0].expires_at > datetime.now(UTC), f"{tomorrow} stored wrong" + + +def test_cli_expiry_accepts_a_full_iso_timestamp(store: KBStore) -> None: + spec = _claim(store, "the spec says tokens rotate every 24 hours") + when = (datetime.now(UTC) + timedelta(days=3)).replace(microsecond=0) + + res = CliRunner().invoke(cli, ["pin", spec, "--expires", when.isoformat()]) + assert res.exit_code == 0, res.output + assert pins.load_pins(store)[0].expires_at == when + + +@pytest.mark.parametrize("spec_text", ["all", "", "not-a-real-spec"]) +def test_cli_expiry_rejects_specs_that_mean_no_bound( + store: KBStore, spec_text: str +) -> None: + """`all` / `""` resolve to "no lower bound", which as an expiry means never. + + Never-expires is already what omitting the flag does, so accepting these + silently would hide a typo rather than honour a request. + """ + spec = _claim(store, "the spec says tokens rotate every 24 hours") + res = CliRunner().invoke(cli, ["pin", spec, "--expires", spec_text]) + + assert res.exit_code != 0 + assert "Error:" in res.output + assert not pins.load_pins(store), "no pin should be written on a bad expiry" + + def test_cli_unknown_artifact_is_a_clean_error(store: KBStore) -> None: """A domain error must render as `Error: ...`, never a traceback.""" res = CliRunner().invoke(cli, ["pin", "never-existed"])