diff --git a/CHANGELOG/v4.md b/CHANGELOG/v4.md index 664c4da48..0bb16f6a1 100644 --- a/CHANGELOG/v4.md +++ b/CHANGELOG/v4.md @@ -17,6 +17,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **`LaneTelemetry.heat_used` reports whether the heat-kernel lane actually fired ([#1162](https://github.com/robotrocketscience/aelfrice/issues/1162)).** Unlike `bm25f_used`, which mirrors the resolved flag, `heat_used` is True only when the heat branch rewrote an L1 ordering — the flag on, a non-stale `GraphEigenbasisCache` supplied, and its rows overlapping the L1 hits. It is written at the two sites that compute the heat map, so a later re-wiring cannot leave the telemetry echoing a stale answer, and it turns "is this lane reachable?" from a grep into a runtime fact. - **Axiomatic retrieval constraints gate the scorer ([#1174](https://github.com/robotrocketscience/aelfrice/issues/1174)).** `tests/test_axiomatic_constraints.py` encodes the Fang, Tao & Zhai (2004) constraints — TFC1, TFC2, TDC, LNC1, LNC2/TF-LNC, QTFC, and stream monotonicity for the anchor field — as executable properties over synthetic in-memory stores, each asserted in both the single-field and the per-field (#1180) scoring modes. These are model-free: they hold for BM25, BM25+, LM-Dirichlet, PL2 and DPH alike, so they survive a scorer rewrite in a way a byte-exact baseline cannot. That is the gap they close — `eval-calibration` can say the ranking *changed*, never that a new ranking is *sane*. Verified to have teeth by mutation rather than assumed: removing length normalisation, removing tf saturation on either lane, flattening `idf`, reverting the #1179 query-term-frequency fix, killing the anchor stream outright, and inverting the BM25 sign convention each fail at least one constraint, while the unmutated tree passes all 19. Three mutations initially escaped, and the reason is worth recording: a non-zero mutation count is not by itself evidence of coverage — *which* test goes red is. The first TFC2 sweep grew the document as it added occurrences, so the diminishing returns it measured came from the length penalty and the test passed with saturation removed entirely; it now substitutes occurrences for filler at constant length. The LNC1 and on-topic-anchor tests each passed with the mechanism they name entirely disabled, because a non-strict inequality is satisfied by "nothing changed at all" and because the supposedly identical twin carried an extra token; the sole red in both cases was a pinned defect record scheduled for deletion. Both now use byte-identical fixtures under strict inequalities. Three constraints do not hold at shipped defaults and are pinned as explicit assertions of current behaviour rather than skipped, so the gate states each defect out loud and a fix has to come here and flip it: QTFC is violated at `DEFAULT_K3 = 0.0` (deliberate, #1179) and holds once `k3 > 0`; off-topic anchor text demotes a cited belief below its uncited twin on the single-field lane and is exactly neutral on the per-field lane (#1180); and a belief on the agent-inferred ingest prior scores 0.144 below one with no evidence at all, so being ingested is a penalty relative to being unknown (#1174). Test-only — no production code path changes. +- **`aelf sweep-feedback --gc` collects the banked deferred-feedback rows ([#1162](https://github.com/robotrocketscience/aelfrice/issues/1162)).** Stores carry six figures of `status='enqueued'` rows from the period when enqueuing was default-on, and the audit-only sweeper cannot act on them — they are a record of exposure, not pending work. `--gc` deletes them and reports the count. It is never implicit: nothing drops a row unless asked, which is why this is a flag rather than a sentinel-gated one-shot on a hot table. Narrow by construction — `applied` and `cancelled` rows are the trail of sweeps that really did run and are left alone — and idempotent, so a second run reports 0. It is also **scoped to exactly the rows the same run reported on**, so the destructive verb cannot outscope the report that justifies it: `--limit` bounds the audit and the deletion together, and eligible rows past it are called out on their own line rather than silently dropped or silently kept. The purge deletes in chunks inside its single transaction, because `--limit` is user-settable and one bind parameter per id would hit SQLite's 32,766-variable cap exactly when an operator widens the run to collect a large backlog. Relatedly, `pending_unmet_grace` is now counted against the grace cutoff instead of inferred as `total - swept`; the subtraction folded genuinely-in-grace rows together with eligible rows past the limit and printed the sum under the former's name. That error was self-correcting while the sweeper drained its page each run, and permanent once it stopped. + - **`aelf:onboard` prompts for the classification model tier ([#1155](https://github.com/robotrocketscience/aelfrice/issues/1155)).** Before dispatching its bulk sentence-classification fan-out, onboard now shows the host's model tiers (low-cost / mid / top) with the per-tier estimated cost for *this* run — the token counts are fixed, only the per-token rate changes — and a plain statement of the trade-off: classification here is short-label typing, and higher-tier models show strongly diminishing returns on quality for it. The user picks a tier; the run defaults to the low-cost tier when the prompt is unanswered or the run is non-interactive/scripted, so unattended onboarding is unaffected. The wording is tier-abstract (the agent names its host's concrete model per tier), so the single shared slash-command source drives both hosts. Slash-command guidance only — no CLI or store change; `--no-subagents` (regex path) still skips the model and the prompt entirely. ### Changed @@ -26,6 +28,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **The heat-kernel authority lane defaults off again ([#1162](https://github.com/robotrocketscience/aelfrice/issues/1162)).** `use_heat_kernel` has resolved `True` since [#154](https://github.com/robotrocketscience/aelfrice/issues/154) flipped it at v2.1.0 on the strength of the #437 reproducibility-harness gate clearing 11/11. The lane never ran. Its scoring branch is guarded on a non-stale `GraphEigenbasisCache` as well as on the flag, and nothing in `src/` constructs one — `retrieve()` accepts a cache as a parameter defaulting to `None` in four signatures, and only tests pass it — so for two minor versions the flag advertised an active lane that was physically unreachable. Flipping the default back is therefore **inert, not a ranking change**: with no eigenbasis every call was already taking the heat-off path, and γ/ζ already run their "no eigenbasis available" branch for the same reason. The full suite passes unchanged, which is the evidence rather than the claim. The lane stays wired behind the same one-line opt-in (env var, kwarg, or TOML key), because [#1113](https://github.com/robotrocketscience/aelfrice/issues/1113) closed graph-theory-as-a-ranking-lever negative and re-enabling this would be a bench proposal rather than a fix. Docs asserting the lane was on are corrected in `CONFIG.md`, `LIMITATIONS.md`, `bayesian_ranking.md` and `BENCHMARKS.md` — the last of which also listed the default-off γ and ζ reranks among the always-on lanes. - **The BM25F index now updates incrementally instead of rebuilding from scratch on every write ([#1199](https://github.com/robotrocketscience/aelfrice/issues/1199)).** `store_generation` bumps on every belief/edge mutation, so the #1135 sidecar was stale at almost every prompt that needed it. Measured over 597 real `user_prompt_submit` fires across 60 sessions on a 45,931-belief store: **86.2% of the fires that actually reach retrieval rebuilt the whole index** (163/189), and the median session rebuilt on *every* such prompt. The naive figure is 27.3% of all fires, but 408 of the 597 are shape-gated before retrieval and never ask for an index, so that denominator credits the sidecar for work it never did. Cold fires ran p50 1839 ms / p95 2499 ms against 246 ms for a sidecar hit, and 96% of the cold path was `BM25Index.build()` re-tokenising and re-stemming every document to absorb a change ratio under 0.1% — roughly 37 writes per retrieval-running prompt against ~45k documents. `BM25Index.update_from` now reuses the rows whose source text is unchanged and tokenises only the rest: **~1.5 s to ~0.4 s on that store, about 4x**, and flat in the size of the change set because what remains is the fixed change-detection scan (~178 ms of SQL plus fingerprinting), not the tokenisation it replaces. Change detection fingerprints the text that was actually indexed rather than reusing `beliefs.content_hash`, which is written by callers and not enforced by the store — keying index validity on it would let one writer's wrong hash silently serve stale retrieval results. The updated index is **identical** to a full rebuild, not merely close: `dl` is copied for reused rows so no float re-rounding creeps in, `df` is counted off the assembled sparsity pattern rather than carried and adjusted, and `idf`/`avgdl` are computed from those at the end; verified field-for-field on the production store and over randomised churn — inserts, edits, soft-deletes, hard-deletes, anchor-edge churn — in both scoring modes. Deletions prune vocabulary terms they orphan, so `n_terms` and every column index after them cannot drift from a fresh build. The path declines and falls back to a full build when it cannot guarantee that: no fingerprints on the base, a different `anchor_weight` or `per_field` (its rows would describe different documents), an empty side, or more than a 50% change ratio. Index serialisation goes to v5 to carry the per-document fingerprints; a blob without them still round-trips and scores, it just cannot seed an update. +- **The deferred-feedback queue stops enqueuing, and its sweeper becomes audit-only ([#1162](https://github.com/robotrocketscience/aelfrice/issues/1162)).** `is_enqueue_on_retrieve_enabled` defaulted `True` and is called inside every `retrieve()`, writing one queue row per surfaced belief — an unbounded write path justified on the grounds that nothing reads a row until the sweeper runs, which was true only because nothing schedules the sweeper. It was also a second, unflagged, default-on route to the posterior bump [#1086](https://github.com/robotrocketscience/aelfrice/issues/1086) had already turned off, having decided that retrieval exposure is deliberately not evidence. Both halves are fixed, because either alone leaves the defect live: enqueuing is now opt-in, and `aelf sweep-feedback` classifies every pending row by the same ladder as before — grace elapsed, explicit signal in window, belief missing, locked, foreign — and reports what it *would* have applied without touching `alpha`, `feedback_history`, or the queue status. That matters because the sweeper had no counterweight: `scoring.decay` / `type_half_life` have no production caller, so a frequently-retrieved belief's evidence grew without bound and its posterior mean walked to 1.0, permanently outranking equal-BM25 peers — the invariant `tests/test_decay_required.py` pre-registers, which passed while production violated it because it exercises the function rather than the pipeline. The audit is read-only rather than mutate-and-drain on purpose: a sweep that consumed its input would report a real number once and zero forever after, which reads as "nothing here" rather than "already spent". `SweepResult` fields are renamed `would_*` to match, since an audit reporting an `applied` count is exactly the ambiguity worth removing, and #1168's check-then-act ordering test is replaced by the stronger structural assertion that the sweep issues no write statement at all. Turning implicit exposure into real feedback again is a separate proposal — it reverses #1086, changes ranking for every user, and needs a bench in front of it. + - **BM25F can score content and anchor text as two separately normalised fields ([#1180](https://github.com/robotrocketscience/aelfrice/issues/1180)).** Default off, behind `[retrieval] bm25f_per_field` / `AELFRICE_BM25F_PER_FIELD`. The shipped lane concatenates each belief's incoming anchor text into its own document and normalises by the *combined* length — the single-field stream-replication approximation, not BM25F. Because the replicas land in the same `dl`, a belief's own content terms are length-penalised in proportion to how much text its citers wrote about it. Measured on a synthetic corpus where two beliefs are identical in everything BM25 can see except that one is cited: when the anchor text never mentions the query term, the cited belief scores **0.27x** the uncited one at 1% corpus-wide anchor density, rising only to 0.56x at 100%. It is punished hardest exactly where production sits, and for something no part of the belief itself did. The new path implements Robertson, Zaragoza & Taylor (2004) properly — `tf~ = Σ_f w_f·tf_f/B_f` with per-stream `B_f`, saturated as `(k1+1)·tf~/(k1+tf~)` — so `anchor_weight` becomes a field weight rather than a replication count, `b_anchor` (`[retrieval] bm25_b_anchor`, default 0.75) is a new tunable, `df` counts a term once across the union of the two streams, and the serialisation format goes to v4. On that same corpus the irrelevant-anchor case is exactly 1.00x at every density. Off by default because this replaces the functional form rather than re-parameterising it: the saturation denominator becomes the constant `k1` instead of `tf + k1·B`, so no choice of constants makes on and off agree once an anchor stream exists, and there is no parity test that could gate the flip — only a bench. **Two corrections to the filed issue.** Its stated formula omits the `(k1+1)` numerator, which would leave every score a factor of 2.5 below the current lane at the shipped `k1 = 1.5` and so break the issue's own acceptance criterion that `w_anchor = 0` recover standard BM25 exactly; the numerator is kept here, and `w_anchor = 0` reproduces the legacy lane's scores exactly up to float32 rounding. The two paths evaluate the same identity in a different order — legacy divides by `(tf + k1·B)`, per-field divides by `B` first and then saturates — so on a 400-belief corpus all 200 probe queries differ, at a relative delta of 2.2e-07 (max absolute 1.9e-06) with identical top-k membership; one query in 200 orders an exact tie differently by a single ulp. Treat a delta of that size at `w_anchor = 0` as rounding, not a regression. And its reason for rejecting the simpler content-only-`dl` fix — that the fix trades a bounded penalty for an *unbounded* boost — does not hold: BM25 saturation caps every variant at `idf·(k1+1)`, measured converging to exactly 1.75x out to a term frequency of 3e9. That fix is still the wrong instrument, because the boost is unearned rather than unbounded, but the conclusion needed a different argument. **What the gating bench must control for:** the anchor stream is sparse to absent in practice. The one real store available for measurement holds 16,454 beliefs and no edges at all, and 60 realistic turns through the production `ingest_turn` path produced 1 anchored belief in 15. Per-field's *boost* is strongly coupled to corpus-wide anchor density (1.08x at 1% anchored, 1.91x at 100%), so a benchmark run over a corpus with little anchor text will report a near-zero delta that says nothing about the scoring change. Any bench of this flag must publish the share of indexed beliefs carrying an incoming anchor, the anchor-stream lengths among those, and the share of eval queries whose gold belief is anchored; a neutral result on a corpus with no anchors is a no-measurement, not a refutation. The *demotion* fix, unlike the boost, is density-independent. - **Benchmark metrics are reported in two families, and the ones the harness cannot compute say so ([#1160](https://github.com/robotrocketscience/aelfrice/issues/1160)).** `aelf bench all` runs no reader, so every adapter hands the joined retrieval context to a scorer written for a model's answer. Token-F1 between ~2000 tokens of context and a three-token gold has precision around 3/2000, which means halving the token budget roughly doubles the reported F1 while retrieving strictly less. Each blob-scoring adapter now also reports `retrieval_quality` — MRR and recall@k over the *ordered* retrieved list, from the new `benchmarks/retrieval_metrics.py`. Because retrieval fills the budget in rank order, a smaller budget truncates the tail, so those metrics are monotone non-decreasing in the budget: moving them requires ranking a relevant belief higher. Metrics that cannot be computed at all now report `n/a` instead of `0.0`, with the reason recorded under `_not_applicable`. `exact_match` (MAB, LongMemEval, AMA-Bench) compares whole normalised strings and can never match a retrieval blob; LoCoMo's adversarial category 5 scores a refusal nothing in this path can produce, and is additionally excluded from `overall_f1`, which becomes the mean over the categories actually scored (`scored_qa` reports how many, beside the unchanged `total_qa`). Reporting these as `0.0` claimed the worst possible score for a measurement never taken, and put a tolerance band around zero where any genuine fix registered as an excursion. `benchmarks/tolerance.py` records an `n/a` leaf as `NOT_APPLICABLE`: tallied and printed, excluded from the rollup, and not counted as evidence anything was measured, so an all-`n/a` run fails as `NO_DATA` rather than passing. The shipped `benchmarks/results/v2.0.0.json` predates the change and still records `0.0` for those leaves; the nightly stays green against it — five leaves resolve to `NOT_APPLICABLE` and the raised `overall_f1` is higher-is-better, so leaving the band warns rather than fails — until the canonical cut is recut. diff --git a/docs/concepts/PHILOSOPHY.md b/docs/concepts/PHILOSOPHY.md index c9864d784..bc82953f8 100644 --- a/docs/concepts/PHILOSOPHY.md +++ b/docs/concepts/PHILOSOPHY.md @@ -152,7 +152,7 @@ The 2,400-token retrieval-API default is a calibrated choice, not an arbitrary o The v1 surface is small. Feedback-driven belief mutation goes through `apply_feedback`, and every lock through one path. When the system misbehaves, there is one place to look. -Being precise about `(α, β)`, since the aspiration and the code have drifted apart before ([#1168](https://github.com/robotrocketscience/aelfrice/issues/1168)): `apply_feedback` is the *primary* writer, not the only one. Three other paths write it, each deliberately and each leaving an audit trail — `deferred_feedback.sweep_deferred_feedback` (the implicit retrieval lane, with its own smaller epsilon and grace window), `clamp_ghosts.clamp_ghost_alpha` (a one-shot migration clamp), and the consolidation dedup pass (which sums existing evidence when collapsing a duplicate group rather than adding new evidence). The invariants that matter hold across all of them: a user lock is a floor no passive signal moves, a federated peer's belief is read-only locally, and every posterior move that is not a merge writes a `feedback_history` row. The posterior write itself is a single atomic SQL increment inside one `BEGIN IMMEDIATE` transaction with its audit row, so concurrent writers cannot lose each other's evidence and the log cannot disagree with the projection. +Being precise about `(α, β)`, since the aspiration and the code have drifted apart before ([#1168](https://github.com/robotrocketscience/aelfrice/issues/1168)): `apply_feedback` is the *primary* writer, not the only one. Two other paths write it, each deliberately and each leaving an audit trail — `clamp_ghosts.clamp_ghost_alpha` (a one-shot migration clamp) and the consolidation dedup pass (which sums existing evidence when collapsing a duplicate group rather than adding new evidence). A third used to: `deferred_feedback.sweep_deferred_feedback`, the implicit retrieval lane, which [#1162](https://github.com/robotrocketscience/aelfrice/issues/1162) made audit-only. It had no counterweight — `scoring.decay` has no production caller — so exposure alone walked a frequently-retrieved belief's posterior upward without bound, and it contradicted [#1086](https://github.com/robotrocketscience/aelfrice/issues/1086), which had already decided that exposure is not evidence. The invariants that matter hold across all of them: a user lock is a floor no passive signal moves, a federated peer's belief is read-only locally, and every posterior move that is not a merge writes a `feedback_history` row. The posterior write itself is a single atomic SQL increment inside one `BEGIN IMMEDIATE` transaction with its audit row, so concurrent writers cannot lose each other's evidence and the log cannot disagree with the projection. The earlier research line had a much bigger surface — twenty-nine MCP tools, `wonder`, `reason`, snapshot/diff. It delivered value but also delivered ambiguity. The rebuild started narrow on purpose; v1.x–v3.x have reintroduced breadth (15 MCP tools at v3.3, plus `/aelf:wonder` / `/aelf:reason` / `/aelf:graph` slash surfaces), each addition gated on evidence — a benchmark, an experiment, a clear case where the existing operations don't suffice. diff --git a/docs/user/COMMANDS.md b/docs/user/COMMANDS.md index d2c64650e..c27aefcb1 100644 --- a/docs/user/COMMANDS.md +++ b/docs/user/COMMANDS.md @@ -55,7 +55,7 @@ DB resolves from `$AELFRICE_DB`, then `/aelfrice/memory.db` when | `bench [--top-k N]` | Run the deterministic 16-belief × 16-query benchmark. Prints a JSON `BenchmarkReport`. | | `bench all --out PATH [--canonical] [--adapters CSV] [--smoke]` | (v2.0+, #437) Reproducibility harness — subprocess each academic-suite adapter (mab, locomo, longmemeval, structmemeval, amabench) at the canonical headline cut and merge into one schema-v2 JSON. `--canonical` asserts the run matches `CANONICAL_INVOCATIONS` (full benchmarks per the 2026-05-06 ratification) and refuses if the cut differs. `--smoke` runs the small SMOKE_INVOCATIONS subset. `--adapters` filters; combined with `--canonical` this refuses (cut mismatch). Returns 0 ok / 1 any error / 2 any skipped_data_missing. | | `tail [--since DUR] [--filter key=value]... [--no-blob] [--no-follow]` | (v1.6+) Live-tail the per-turn hook audit log at `/aelfrice/hook_audit.jsonl`. Per fire: a header line (time, hook, tokens, latency, `L0×N L1×M`) plus one indented snippet line per injected belief. `--no-blob` suppresses snippet bodies; `--no-follow` dumps and exits; `--filter` matches fields like `hook=user_prompt_submit` / `lane=L0` (repeatable). See [hook-injection-audit.md](../design/hook-injection-audit.md). | -| `sweep-feedback` | (v1.6+) Run the deferred-feedback sweeper once (#191). Enqueue-on-retrieve is default-on, but the queue has no automatic consumer — this verb is the manual one-shot pass that applies `+ε` (default 0.05) α per enqueued exposure past its grace window. Note this is a *separate*, legacy exposure-as-evidence path: the primary hook retrieval path is audit-only by default since #1086 (see `AELFRICE_EXPOSURE_UPDATES_POSTERIOR`), and #1091 flagged this sweep for the same treatment. | +| `sweep-feedback` | (v1.6+) **Audit** the deferred-feedback queue (#191). Reports how many exposures past their grace window *would* have received `+ε` (default 0.05) α, and what that would total — and changes nothing. #1162 gave this sweep the treatment #1091 flagged it for, so both exposure-as-evidence paths are now audit-only and agree; enqueue-on-retrieve is opt-in in the same change. Because nothing is consumed the numbers are repeatable rather than draining to zero on the first run. `--gc` deletes the banked `enqueued` rows the sweep can no longer act on and reports the count; it is the one destructive action and never implicit. | | `eval [--corpus PATH]` | Run the relevance-calibration harness (P@K / ROC-AUC / Spearman ρ) on a synthetic corpus. Prints the calibration block; exit 0 on success. | | `clamp-ghosts [--threshold F] [--target F] [--apply] [--limit N]` | **Hidden / advanced.** One-shot repair tool for stores migrated from pre-v1.0 schemas. Identifies belief rows whose α is inflated above prior yet have zero `feedback_history` and zero `belief_corroborations` entries (audit-trail-less ghosts; α floor `--threshold`, default 4.0). `--target` (default 4.0, must be ≤ threshold) is the α value clamped down to; `--limit` caps rows per call. Default dry-run; `--apply` writes the UPDATE plus a negative-valence `feedback_history` row inside one transaction so the clamp is reversible and idempotent. | | `scan-derivation --reference PATH [--threshold F] [--n N] [PATH ...]` | (v3.0+, [#681](https://github.com/robotrocketscience/aelfrice/issues/681)) N-gram Jaccard similarity gate against a reference document. Reads each PATH (or stdin via `-` / no args), prints `MATCH [score] label: excerpt` or `clean [score] label`, exits 0 (all clean) / 1 (one or more matched) / 2 (reference unreadable). Designed to drop into a git pre-commit / pre-push hook. Defaults: 3-gram windows, threshold 0.6. | diff --git a/src/aelfrice/cli.py b/src/aelfrice/cli.py index 11be631a9..d7a33891d 100644 --- a/src/aelfrice/cli.py +++ b/src/aelfrice/cli.py @@ -7051,12 +7051,19 @@ def _print_doctor_session_ring(out: object) -> None: def _cmd_sweep_feedback(args: argparse.Namespace, out: object) -> int: - """Process the deferred-feedback queue (#191). + """Audit the deferred-feedback queue (#191, audit-only since #1162). - Applies +epsilon to the alpha of each belief whose retrieval- - exposure row has cleared its grace window without a contradicting - explicit-feedback event. Cancels rows where an explicit signal - landed within the grace window. Idempotent. + Reports how many rows would have received `+epsilon` under the + pre-#1162 sweeper and what that would have totalled, and mutates + nothing — no alpha, no feedback_history, no queue status. Because + it consumes nothing the numbers are repeatable rather than + draining to zero on the first run. + + `--gc` is the one destructive action here and it is never implicit: + it deletes the banked `status='enqueued'` rows the audit just + reported on — exactly those, so the report cannot describe less + than the deletion — and prints the count. Rows past `--limit` are + left alone and called out separately. Exits 0 unless `--strict` is passed and an exception escapes. Without `--strict`, errors are logged to stderr and the command @@ -7081,6 +7088,7 @@ def _cmd_sweep_feedback(args: argparse.Namespace, out: object) -> int: limit = int(args.limit) if args.limit is not None else 10_000 store = _open_store() + purged: int | None = None try: result = sweep_deferred_feedback( store, @@ -7088,6 +7096,15 @@ def _cmd_sweep_feedback(args: argparse.Namespace, out: object) -> int: epsilon=eps, limit=limit, ) + # After the audit, and scoped to exactly the rows it classified. + # Deleting every enqueued row instead would give the destructive + # verb wider scope than the report justifying it: on a six-figure + # queue the audit describes the first `limit` rows and the purge + # would remove all of them. + if getattr(args, "gc", False): + purged = store.purge_enqueued_deferred_feedback( + result.audited_row_ids + ) except Exception as exc: # noqa: BLE001 - cron-safe by default print(f"aelf sweep-feedback: {exc}", file=sys.stderr) if getattr(args, "strict", False): @@ -7100,14 +7117,31 @@ def _cmd_sweep_feedback(args: argparse.Namespace, out: object) -> int: pass print( - f"sweep-feedback: applied={result.applied} " - f"cancelled={result.cancelled} " - f"skipped_no_belief={result.skipped_no_belief} " + f"sweep-feedback (audit-only, no alpha changed): " + f"would_apply={result.would_apply} " + f"would_cancel={result.would_cancel} " + f"would_skip_no_belief={result.would_skip_no_belief} " + f"would_skip_locked={result.would_skip_locked} " + f"would_skip_foreign={result.would_skip_foreign} " f"pending_in_grace={result.pending_unmet_grace} " + f"pending_beyond_limit={result.pending_beyond_limit} " + f"alpha_withheld={result.alpha_withheld:.4f} " f"epsilon={result.epsilon_used} " f"grace_seconds={result.grace_seconds_used}", file=out, # type: ignore[arg-type] ) + if purged is not None: + print( + f"sweep-feedback: --gc deleted {purged} banked enqueued row(s)", + file=out, # type: ignore[arg-type] + ) + if result.pending_beyond_limit: + print( + f"sweep-feedback: {result.pending_beyond_limit} eligible row(s) " + f"past --limit ({limit}) were neither reported on above nor " + "collected; re-run, or raise --limit to widen both together", + file=out, # type: ignore[arg-type] + ) return 0 @@ -8950,9 +8984,9 @@ def _positive_int(s: str) -> int: p_sweep_feedback = sub.add_parser( "sweep-feedback", help=( - "process deferred retrieval-exposure feedback queue (#191): " - "apply +epsilon to beliefs whose grace window elapsed without " - "a contradicting explicit signal" + "audit the deferred retrieval-exposure feedback queue (#191): " + "report what the pre-#1162 sweeper would have applied. Changes " + "no belief; pass --gc to drop the banked rows" ), ) p_sweep_feedback.add_argument( @@ -8980,6 +9014,17 @@ def _positive_int(s: str) -> int: "--strict", action="store_true", help="exit non-zero on any exception (default: log + exit 0 for cron)", ) + p_sweep_feedback.add_argument( + "--gc", action="store_true", + help=( + "delete the banked status='enqueued' rows this run reported " + "on (#1162). The audit-only sweeper cannot act on these, so " + "they are a record rather than pending work. Scoped to the " + "audited rows, so --limit bounds the deletion and the report " + "together; leaves 'applied' and 'cancelled' rows — the trail " + "of sweeps that did run — alone. Idempotent" + ), + ) p_sweep_feedback.set_defaults(func=_cmd_sweep_feedback) # Hidden: one-shot maintenance for pre-migration ghost-α rows diff --git a/src/aelfrice/deferred_feedback.py b/src/aelfrice/deferred_feedback.py index 5e86be97b..568f04162 100644 --- a/src/aelfrice/deferred_feedback.py +++ b/src/aelfrice/deferred_feedback.py @@ -7,6 +7,13 @@ applies `+epsilon` to each belief whose grace window has elapsed without an explicit correction or contradiction landing on it. +**Enqueuing is opt-in since #1162** (`[implicit_feedback] +enqueue_on_retrieve`, default False). It defaulted on, writing a row +per surfaced belief inside every `retrieve()`, on the reasoning that +nothing consumes a row until the sweeper runs — which was true only +because nothing schedules the sweeper. See +`is_enqueue_on_retrieve_enabled` for the rest of that argument. + Contracts (see issue #191 for full spec): * `T_grace`: enqueue_at + T_grace must be <= now before a row is @@ -75,31 +82,62 @@ @dataclass class SweepResult: - """Outcome of one `sweep_deferred_feedback` invocation. - - `applied` and `cancelled` count rows whose status transitioned - during this call. `skipped_no_belief` counts rows whose belief_id - no longer resolves (the belief was deleted between enqueue and - sweep) — those rows are marked cancelled so the queue drains. - - `skipped_locked` and `skipped_foreign` (#1168) count rows dropped - because the belief carries a user lock, or is owned by a federated - peer and therefore read-only locally. Both are also counted in - `cancelled`: the row is drained rather than left enqueued, so an - ineligible belief cannot build a backlog that all lands at once if - it later becomes eligible. + """Outcome of one `sweep_deferred_feedback` audit (#1162). + + **Every field is a projection, not a record of work done.** The + sweep writes nothing: no `alpha`, no `feedback_history` row, no + queue-status transition. `would_apply` is the count of rows that + *would* have received `+epsilon` under the pre-#1162 sweeper, and + `alpha_withheld` is what that would have totalled. + + Naming them `would_*` is the point. The previous shape called them + `applied` / `cancelled`, and an audit-only sweeper reporting an + `applied` count is exactly the ambiguity that lets "the sweep ran + and reported 12k applied" be read as a mutation that happened. + + `would_cancel` counts rows an explicit signal landed on inside the + grace window — under the old sweeper those drained without a + posterior change. `would_skip_no_belief`, `would_skip_locked` and + `would_skip_foreign` (#1168) are rows whose belief no longer + resolves, carries a user lock, or belongs to a federated peer; all + three are also counted in `would_cancel`, matching how the mutating + sweeper drained them. + + Because nothing is written, the audit is repeatable: running it + twice reports the same numbers rather than draining to zero. That + is what makes the count usable as a standing measurement of how + much implicit signal the store is sitting on. """ - applied: int = 0 - cancelled: int = 0 - skipped_no_belief: int = 0 - skipped_locked: int = 0 - skipped_foreign: int = 0 + would_apply: int = 0 + would_cancel: int = 0 + would_skip_no_belief: int = 0 + would_skip_locked: int = 0 + would_skip_foreign: int = 0 + # Rows whose grace window has NOT elapsed. Counted directly, not + # inferred by subtracting this page from the queue total — that + # subtraction labels everything past `limit` as "still in grace", + # which is false for any queue bigger than one page and, now that + # nothing drains, permanently so. pending_unmet_grace: int = 0 + # Rows that ARE eligible but fell outside this pass's `limit`. + # Reported separately because they are the gap between what the + # audit describes and what the queue holds: `alpha_withheld` and + # every `would_*` count below is a figure for the audited page, not + # for the store, whenever this is non-zero. + pending_beyond_limit: int = 0 + alpha_withheld: float = 0.0 epsilon_used: float = 0.0 grace_seconds_used: int = 0 - applied_belief_ids: list[str] = field(default_factory=list) - cancelled_belief_ids: list[str] = field(default_factory=list) + would_apply_belief_ids: list[str] = field(default_factory=list) + would_cancel_belief_ids: list[str] = field(default_factory=list) + # Queue row ids this pass actually classified. `--gc` deletes + # exactly these, so the report and the deletion cannot diverge. + audited_row_ids: list[int] = field(default_factory=list) + # Permanently False, asserted rather than documented. A future + # change that reintroduces writes has to flip this and face the + # test that pins it. + mutated: bool = False # --- Time helpers ------------------------------------------------------- @@ -202,9 +240,24 @@ def resolve_epsilon( def is_enqueue_on_retrieve_enabled( explicit: bool | None = None, *, start: Path | None = None ) -> bool: - """Env > kwarg > TOML > default True. Default-on because the queue is - additive (no consumer reads it until the sweeper runs); operators can - flip it off without losing any other functionality.""" + """Env > kwarg > TOML > default **False** (#1162). + + This defaulted True on the argument that the queue is additive — + nothing reads a row until the sweeper runs. That held only because + the sweeper is a manual command nothing schedules, which is an + accident of deployment rather than a design property. Meanwhile the + call sits inside every `retrieve()` and writes a row per surfaced + belief, so a store banks rows without bound. + + It also ran against a decision already taken: #1086 set + `_exposure_updates_posterior()` default False — retrieval exposure + is deliberately not posterior evidence. This queue was a second, + unflagged, default-on route to the same posterior bump. + + Enqueuing is still a one-line opt-in for anyone measuring exposure. + What it can no longer do is feed `alpha`: the sweeper is audit-only + since #1162, so the rows are a record, not a pending mutation. + """ raw_env = os.environ.get(ENV_ENQUEUE) if raw_env is not None: norm = raw_env.strip().lower() @@ -217,7 +270,7 @@ def is_enqueue_on_retrieve_enabled( toml_v = _read_toml_value(ENQUEUE_KEY, start=start) if isinstance(toml_v, bool): return toml_v - return True + return False # --- Enqueue path ------------------------------------------------------- @@ -261,26 +314,45 @@ def sweep_deferred_feedback( limit: int = 10_000, config_start: Path | None = None, ) -> SweepResult: - """Process pending queue rows whose grace window has elapsed. - - For each pending row with `enqueued_at <= now - grace_seconds`: - - * If `feedback_history` has an entry for this belief whose source - is not `RETRIEVAL_DRIVEN_FEEDBACK_SOURCE` and whose created_at - is in `[enqueued_at, now]`, mark `cancelled` (no posterior - change). This covers explicit user feedback AND contradiction - tiebreaker events in one query. - * Else apply `+epsilon` to belief.alpha, write a feedback_history - row with source=RETRIEVAL_DRIVEN_FEEDBACK_SOURCE, and mark - `applied`. The three writes share one transaction so a crash - mid-row leaves the queue row `enqueued` and the alpha - unchanged — re-run applies once. - * If the belief no longer exists, mark `cancelled` and count - toward `skipped_no_belief` so the queue drains. - - Idempotent: only `enqueued` rows are touched; re-running the - sweeper over the resulting state is a no-op for the rows it - already processed.""" + """Audit the deferred-feedback queue. **Writes nothing** (#1162). + + Classifies every pending row exactly as the mutating sweeper did — + grace elapsed, explicit signal in window, belief missing, locked, + or foreign — and reports what it *would* have applied. No `alpha` + moves, no `feedback_history` row is written, no queue status + changes. + + The sweeper used to apply `+epsilon` per eligible row. Two things + made that unsafe rather than merely unused: + + * **No counterweight.** `scoring.decay` / `type_half_life` have + no production caller, so a frequently-retrieved belief's alpha + grows without bound and its posterior mean walks to 1.0, + permanently outranking equal-BM25 peers. + * **A banked backlog.** Enqueuing was default-on inside every + `retrieve()`, so real stores carry six figures of pending rows. + One invocation would have fired the entire backlog at once — + which is why leaving a mutating sweeper in place while merely + flipping the enqueue default would not have been enough. + + Making the audit read-only rather than "mutate but drain" is + deliberate: a sweeper that consumed the rows would report a + non-zero count once and zero forever after, which reads as "there + is nothing here" rather than "this was already spent". Nothing is + consumed, so the number stays honest and the audit is repeatable. + + `limit` bounds the per-row classification, not the queue counts. + When it bites, `pending_beyond_limit` is non-zero and every + `would_*` figure describes the audited page rather than the store — + raise `limit` to widen both together. `audited_row_ids` records + exactly what this pass looked at, so a caller collecting the + backlog can delete precisely what was reported on. + + Turning implicit exposure into real feedback again is a separate + proposal — it reverses #1086, changes ranking for every user, and + needs a bench in front of it. It is not a matter of re-enabling + this function. + """ grace_eff = ( grace_seconds if grace_seconds is not None @@ -305,106 +377,66 @@ def sweep_deferred_feedback( cutoff_iso=cutoff_iso, limit=limit ) - # Count rows still in their grace window separately. + # Rows still inside their grace window, counted against the cutoff + # rather than derived by subtraction. The subtraction form was + # `enqueued_total - len(pending)`, which folds two disjoint + # populations — genuinely-in-grace rows and eligible rows past the + # `limit` — into one number and prints it under the former's name. + # Under the mutating sweeper that error was transient: a run drained + # its page, the total fell, and the next run saw the remainder. + # Audit-only makes it permanent, because nothing drains and the same + # page is re-reported forever. by_status = store.count_deferred_feedback_by_status() enqueued_total = by_status.get("enqueued", 0) - result.pending_unmet_grace = max(0, enqueued_total - len(pending)) - - conn = store._conn # noqa: SLF001 - intentional, atomic per row + result.pending_unmet_grace = ( + store.count_enqueued_deferred_feedback_in_grace(cutoff_iso=cutoff_iso) + ) + result.pending_beyond_limit = max( + 0, enqueued_total - result.pending_unmet_grace - len(pending) + ) for row_id, belief_id, enqueued_at, _event_type in pending: - # Single explicit transaction per row, taken BEFORE the eligibility - # reads. `BEGIN IMMEDIATE` acquires the write lock up-front, so the - # belief cannot be locked, deleted, or reassigned to a peer between - # the check and the write — a check-then-act window that would let - # +epsilon land on a belief the checks just rejected (#1168). - conn.execute("BEGIN IMMEDIATE") - try: - belief = store.get_belief(belief_id) - - # #1168 AC4. This sweeper writes alpha directly rather than - # going through `apply_feedback` — deliberately, because it - # owns this transaction and the queue-status bookkeeping. What - # it must not do is skip the invariants that endpoint enforces: - # - # * the lock floor. A retrieval-driven +epsilon was landing - # on user locks, which docs/user/LIMITATIONS.md and - # PRIVACY.md both promise cannot happen. Nothing is owed to - # a belief already held at ground truth. - # * federation ownership (#655). A foreign belief id is - # read-only through the local DB. - # - # Each drains the queue row rather than leaving it enqueued, so - # an ineligible belief cannot accumulate a backlog that would - # all land at once if it later becomes eligible. - skip_counter: str | None = None - if belief is None: - skip_counter = "skipped_no_belief" - elif belief.lock_level == LOCK_USER: - skip_counter = "skipped_locked" - else: - try: - store.assert_local_ownership(belief_id) - except ValueError: - skip_counter = "skipped_foreign" - - if skip_counter is not None: - conn.execute( - "UPDATE deferred_feedback_queue " - "SET status='cancelled', applied_at=? WHERE id=?", - (now_iso, row_id), - ) - conn.execute("COMMIT") - setattr( - result, skip_counter, - getattr(result, skip_counter) + 1, - ) - result.cancelled += 1 - result.cancelled_belief_ids.append(belief_id) - continue - - cancelled = store.has_explicit_feedback_in_window( - belief_id, - window_start_iso=enqueued_at, - window_end_iso=now_iso, - retrieval_source=RETRIEVAL_DRIVEN_FEEDBACK_SOURCE, - ) - - if cancelled: - conn.execute( - "UPDATE deferred_feedback_queue " - "SET status='cancelled', applied_at=? WHERE id=?", - (now_iso, row_id), - ) - conn.execute("COMMIT") - result.cancelled += 1 - result.cancelled_belief_ids.append(belief_id) - else: - conn.execute( - "UPDATE beliefs SET alpha = alpha + ? WHERE id = ?", - (eps_eff, belief_id), - ) - conn.execute( - "INSERT INTO feedback_history " - "(belief_id, valence, source, created_at) " - "VALUES (?, ?, ?, ?)", - ( - belief_id, - eps_eff, - RETRIEVAL_DRIVEN_FEEDBACK_SOURCE, - now_iso, - ), - ) - conn.execute( - "UPDATE deferred_feedback_queue " - "SET status='applied', applied_at=? WHERE id=?", - (now_iso, row_id), - ) - conn.execute("COMMIT") - result.applied += 1 - result.applied_belief_ids.append(belief_id) - except Exception: - conn.execute("ROLLBACK") - raise - + result.audited_row_ids.append(row_id) + # No transaction, and no BEGIN IMMEDIATE. The mutating sweeper + # took the write lock before its eligibility reads to close the + # check-then-act window #1168 found; with nothing written there + # is no window to close, and holding a write lock across an + # audit of a six-figure queue would block ingest for no reason. + belief = store.get_belief(belief_id) + + # Same ineligibility ladder the mutating sweeper applied, in the + # same order, so the projection describes that sweeper rather + # than a simplified model of it. + skip_counter: str | None = None + if belief is None: + skip_counter = "would_skip_no_belief" + elif belief.lock_level == LOCK_USER: + skip_counter = "would_skip_locked" + else: + try: + store.assert_local_ownership(belief_id) + except ValueError: + skip_counter = "would_skip_foreign" + + if skip_counter is not None: + setattr(result, skip_counter, getattr(result, skip_counter) + 1) + result.would_cancel += 1 + result.would_cancel_belief_ids.append(belief_id) + continue + + if store.has_explicit_feedback_in_window( + belief_id, + window_start_iso=enqueued_at, + window_end_iso=now_iso, + retrieval_source=RETRIEVAL_DRIVEN_FEEDBACK_SOURCE, + ): + result.would_cancel += 1 + result.would_cancel_belief_ids.append(belief_id) + else: + result.would_apply += 1 + result.would_apply_belief_ids.append(belief_id) + + # Rounded at the assignment: this is a report figure, and + # `12 * 0.05` otherwise prints as 0.6000000000000001. + result.alpha_withheld = round(result.would_apply * eps_eff, 6) return result diff --git a/src/aelfrice/store.py b/src/aelfrice/store.py index cc563c3ae..02a61aeee 100644 --- a/src/aelfrice/store.py +++ b/src/aelfrice/store.py @@ -5004,6 +5004,78 @@ def count_deferred_feedback_by_status(self) -> dict[str, int]: ) return {str(r["status"]): int(r["n"]) for r in cur.fetchall()} + def count_enqueued_deferred_feedback_in_grace( + self, *, cutoff_iso: str + ) -> int: + """Count `status='enqueued'` rows whose grace window has NOT + elapsed, i.e. `enqueued_at > cutoff_iso`. + + Counted directly rather than inferred by subtracting the swept + page from the total. That subtraction attributes every row past + the caller's `LIMIT` to the grace window, which is false for any + queue larger than one page — and permanently so now that the + sweeper is audit-only and never drains. + """ + cur = self._conn.execute( + """ + SELECT COUNT(*) AS n + FROM deferred_feedback_queue + WHERE status = 'enqueued' AND enqueued_at > ? + """, + (cutoff_iso,), + ) + row = cur.fetchone() + return int(row["n"]) if row else 0 + + def purge_enqueued_deferred_feedback(self, row_ids: Sequence[int]) -> int: + """Delete the given `status='enqueued'` queue rows; return the + count actually removed. + + The #1162 backlog collector. Enqueuing was default-on inside + every `retrieve()`, so stores carry six figures of pending rows + that the audit-only sweeper can no longer act on. They are a + record of exposure, not a pending mutation, and this drops them. + + **Takes explicit row ids rather than deleting every enqueued + row.** The caller reports on a bounded page of the queue, and a + destructive verb must not have wider scope than the report that + justifies it — an unbounded delete under a report of the first + `limit` rows would, on a six-figure queue, describe a few per + cent of what it removed. + + Deletes in `_param_chunks`-sized batches inside one transaction: + `limit` is user-settable, so an id list can exceed SQLite's bind + cap and a single `IN (...)` would fail exactly when an operator + widens the run to collect a large backlog. + + The `status = 'enqueued'` predicate is kept as a guard even + though the ids come from a query that already filtered on it: + `applied` and `cancelled` rows are the audit trail of sweeps + that really did run, and no id list should be able to take one. + Idempotent — re-deleting a removed id returns 0. + """ + ids = list(row_ids) + if not ids: + return 0 + # Chunked because `limit` is user-settable and the sweeper's own + # guidance is to raise it: one bind parameter per id would hit + # SQLITE_LIMIT_VARIABLE_NUMBER (32,766 on this connection) and + # turn "widen the audit to see the whole queue" into an opaque + # `too many SQL variables`. Chunking inside the single + # transaction, so atomicity and the audit-trail guarantee below + # are unchanged — either every id goes or none does. + removed = 0 + with self.transaction(): + for part in _param_chunks(ids): + placeholders = ",".join("?" * len(part)) + cur = self._conn.execute( + "DELETE FROM deferred_feedback_queue " + f"WHERE status = 'enqueued' AND id IN ({placeholders})", + tuple(part), + ) + removed += int(cur.rowcount or 0) + return removed + # --- Aggregations (used by aelf:health) ------------------------------ def count_beliefs(self) -> int: diff --git a/tests/test_cli_sweep_feedback.py b/tests/test_cli_sweep_feedback.py index bf4b3780e..71e5d9a53 100644 --- a/tests/test_cli_sweep_feedback.py +++ b/tests/test_cli_sweep_feedback.py @@ -64,10 +64,18 @@ def test_sweep_feedback_empty_queue_exits_zero(store_path: Path) -> None: s.close() code, output = _run() assert code == 0 - assert "applied=0 cancelled=0" in output + assert "would_apply=0 would_cancel=0" in output -def test_sweep_feedback_applies_with_grace_zero(store_path: Path) -> None: +def test_sweep_feedback_reports_but_changes_nothing(store_path: Path) -> None: + """The #1162 acceptance criterion, both halves. + + Running the sweeper on a store with eligible rows must change no + belief's alpha — and must still *report* a non-zero eligible count. + The second half is the negative control: an audit-only sweeper that + quietly reported zero would satisfy the first assertion while + hiding that the queue had stopped being read at all. + """ s = MemoryStore(str(store_path)) s.insert_belief(_mk("b1")) enqueue_retrieval_exposures(s, ["b1"], now="2026-04-28T00:00:00Z") @@ -75,18 +83,103 @@ def test_sweep_feedback_applies_with_grace_zero(store_path: Path) -> None: # grace=0 means everything is immediately eligible. code, output = _run("--grace-seconds", "0", "--epsilon", "0.10") assert code == 0 - assert "applied=1" in output + assert "would_apply=1" in output + assert "alpha_withheld=0.1000" in output + assert "no alpha changed" in output s2 = MemoryStore(str(store_path)) try: b = s2.get_belief("b1") - assert b is not None and b.alpha == 1.10 - events = s2.list_feedback_events(belief_id="b1") - assert any( - e.source == RETRIEVAL_DRIVEN_FEEDBACK_SOURCE for e in events - ) + assert b is not None and b.alpha == 1.0, "the sweeper moved alpha" + assert s2.list_feedback_events(belief_id="b1") == [] + # The row is still enqueued: nothing was consumed, so a second + # run reports the same number rather than draining to zero. + assert s2.count_deferred_feedback_by_status() == {"enqueued": 1} + finally: + s2.close() + code2, output2 = _run("--grace-seconds", "0", "--epsilon", "0.10") + assert code2 == 0 + assert "would_apply=1" in output2 + + +def test_sweep_feedback_gc_drops_only_the_banked_rows( + store_path: Path, +) -> None: + """--gc is the one destructive action, and it is never implicit.""" + s = MemoryStore(str(store_path)) + s.insert_belief(_mk("b1")) + s.insert_belief(_mk("b2")) + enqueue_retrieval_exposures(s, ["b1", "b2"], now="2026-04-28T00:00:00Z") + # An 'applied' row from a sweep that really did run, back when the + # sweeper mutated. That trail must survive the collector. + s._conn.execute( # noqa: SLF001 - fixture reaches for the trail directly + "UPDATE deferred_feedback_queue SET status='applied' " + "WHERE belief_id = 'b2'" + ) + s._conn.commit() # noqa: SLF001 + s.close() + + code, output = _run("--grace-seconds", "0") + assert code == 0 + assert "--gc" not in output, "gc ran without being asked" + s_mid = MemoryStore(str(store_path)) + assert s_mid.count_deferred_feedback_by_status() == { + "enqueued": 1, "applied": 1, + } + s_mid.close() + + code, output = _run("--grace-seconds", "0", "--gc") + assert code == 0 + assert "deleted 1 banked enqueued row(s)" in output + s2 = MemoryStore(str(store_path)) + try: + assert s2.count_deferred_feedback_by_status() == {"applied": 1} + finally: + s2.close() + + # Idempotent. + code, output = _run("--grace-seconds", "0", "--gc") + assert code == 0 + assert "deleted 0 banked enqueued row(s)" in output + + +def test_gc_deletes_only_what_the_run_reported_on(store_path: Path) -> None: + """The destructive verb must not outscope the report that justifies + it. With `--limit 3` over 8 eligible rows, the audit describes 3 and + `--gc` removes those 3 — not all 8 — and the remainder is called out + rather than silently dropped or silently kept. + """ + s = MemoryStore(str(store_path)) + for i in range(8): + s.insert_belief(_mk(f"b{i}")) + enqueue_retrieval_exposures( + s, [f"b{i}" for i in range(8)], now="2026-04-28T00:00:00Z" + ) + s.close() + + code, output = _run("--grace-seconds", "0", "--limit", "3", "--gc") + assert code == 0 + assert "would_apply=3" in output + assert "deleted 3 banked enqueued row(s)" in output + assert "pending_beyond_limit=5" in output + assert "5 eligible row(s) past --limit (3)" in output + + s2 = MemoryStore(str(store_path)) + try: + assert s2.count_deferred_feedback_by_status() == {"enqueued": 5} finally: s2.close() + # Re-running reaches the next page rather than re-reporting the same + # one — the property the audit-only design is supposed to buy. + code, output = _run("--grace-seconds", "0", "--limit", "3", "--gc") + assert code == 0 + assert "deleted 3 banked enqueued row(s)" in output + s3 = MemoryStore(str(store_path)) + try: + assert s3.count_deferred_feedback_by_status() == {"enqueued": 2} + finally: + s3.close() + def test_sweep_feedback_strict_flag_propagates_failure( store_path: Path, monkeypatch: pytest.MonkeyPatch diff --git a/tests/test_implicit_feedback.py b/tests/test_implicit_feedback.py index b4679eafc..fb3694ba9 100644 --- a/tests/test_implicit_feedback.py +++ b/tests/test_implicit_feedback.py @@ -23,6 +23,8 @@ from __future__ import annotations import os + +import pytest from pathlib import Path from aelfrice.deferred_feedback import ( @@ -104,7 +106,16 @@ def test_schema_creates_dfq_indexes() -> None: # --- AC2: retrieve() enqueues ------------------------------------------- -def test_retrieve_enqueues_one_row_per_surfaced_belief() -> None: +def test_retrieve_does_not_enqueue_by_default() -> None: + """#1162. The counterpart to the row below: with no opt-in, a + retrieval writes nothing to the queue.""" + s = _store(_mk("b1", "apple banana"), _mk("b2", "cherry")) + retrieve(s, "apple") + assert s.count_deferred_feedback_by_status() == {} + + +def test_retrieve_enqueues_one_row_per_surfaced_belief(monkeypatch) -> None: + monkeypatch.setenv("AELFRICE_IMPLICIT_FEEDBACK_ENQUEUE", "1") s = _store(_mk("b1", "apple banana"), _mk("b2", "cherry")) out = retrieve(s, "apple") assert {b.id for b in out} == {"b1"} @@ -130,6 +141,7 @@ def test_empty_query_does_not_enqueue() -> None: def test_enqueue_failure_does_not_break_retrieve(monkeypatch, capsys) -> None: + monkeypatch.setenv("AELFRICE_IMPLICIT_FEEDBACK_ENQUEUE", "1") s = _store(_mk("b1", "apple")) import aelfrice.deferred_feedback as df def boom(*a, **k): @@ -140,20 +152,32 @@ def boom(*a, **k): assert "deferred-feedback enqueue failed" in capsys.readouterr().err -# --- AC4: applied path (+epsilon) --------------------------------------- +# --- AC4: eligible path, audit-only since #1162 ------------------------- -def test_sweep_applies_epsilon_after_grace() -> None: +def test_sweep_reports_the_eligible_row_and_moves_no_alpha() -> None: + """The #1162 acceptance criterion in one test, both halves. + + A sweep over a store with pending rows must change no belief's + alpha, and must still report a non-zero eligible count. Dropping + either half leaves a passing test: an audit that reports zero + satisfies the alpha assertion while hiding that the queue stopped + being read, and a mutating sweeper satisfies the count. + """ s = _store(_mk("b1")) enqueue_retrieval_exposures(s, ["b1"], now=T0) r = sweep_deferred_feedback( s, now=T_AFTER_GRACE, grace_seconds=1800, epsilon=0.05 ) - assert r.applied == 1 - assert r.cancelled == 0 + assert r.would_apply == 1 + assert r.would_cancel == 0 + assert r.alpha_withheld == pytest.approx(0.05) + assert r.mutated is False b = s.get_belief("b1") - assert b is not None and b.alpha == 1.05 - assert s.count_deferred_feedback_by_status() == {"applied": 1} + assert b is not None and b.alpha == 1.0 + # Nothing consumed: the row is still enqueued, not marked applied. + assert s.count_deferred_feedback_by_status() == {"enqueued": 1} + assert s.list_feedback_events(belief_id="b1") == [] def test_sweep_skips_rows_inside_grace_window() -> None: @@ -162,8 +186,9 @@ def test_sweep_skips_rows_inside_grace_window() -> None: r = sweep_deferred_feedback( s, now=T_INSIDE_GRACE, grace_seconds=1800, epsilon=0.05 ) - assert r.applied == 0 + assert r.would_apply == 0 assert r.pending_unmet_grace == 1 + assert r.alpha_withheld == 0.0 b = s.get_belief("b1") assert b is not None and b.alpha == 1.0 assert s.count_deferred_feedback_by_status() == {"enqueued": 1} @@ -181,8 +206,8 @@ def test_explicit_feedback_in_grace_window_cancels_implicit() -> None: r = sweep_deferred_feedback( s, now=T_AFTER_GRACE, grace_seconds=1800, epsilon=0.05 ) - assert r.applied == 0 - assert r.cancelled == 1 + assert r.would_apply == 0 + assert r.would_cancel == 1 b = s.get_belief("b1") assert b is not None and b.alpha == 1.0 @@ -200,8 +225,8 @@ def test_contradiction_tiebreaker_event_in_grace_cancels() -> None: r = sweep_deferred_feedback( s, now=T_AFTER_GRACE, grace_seconds=1800, epsilon=0.05 ) - assert r.applied == 0 - assert r.cancelled == 1 + assert r.would_apply == 0 + assert r.would_cancel == 1 def test_explicit_feedback_outside_grace_does_not_cancel() -> None: @@ -215,8 +240,8 @@ def test_explicit_feedback_outside_grace_does_not_cancel() -> None: r = sweep_deferred_feedback( s, now=T_AFTER_GRACE, grace_seconds=1800, epsilon=0.05 ) - assert r.applied == 1 - assert r.cancelled == 0 + assert r.would_apply == 1 + assert r.would_cancel == 0 def test_belief_deleted_between_enqueue_and_sweep_cascades_queue_row() -> None: @@ -230,37 +255,49 @@ def test_belief_deleted_between_enqueue_and_sweep_cascades_queue_row() -> None: r = sweep_deferred_feedback( s, now=T_AFTER_GRACE, grace_seconds=1800, epsilon=0.05 ) - assert r.applied == 0 - assert r.cancelled == 0 + assert r.would_apply == 0 + assert r.would_cancel == 0 assert s.count_deferred_feedback_by_status() == {} -# --- AC6: audit-log event type distinct --------------------------------- +# --- AC6: the sweep leaves no audit row at all -------------------------- -def test_applied_row_writes_distinctive_audit_source() -> None: +def test_sweep_writes_no_feedback_history_row() -> None: + """The inverse of the pre-#1162 assertion, and the more useful one. + + An audit-only sweep must not leave a `RETRIEVAL_DRIVEN_FEEDBACK_SOURCE` + row behind, because such a row is what every downstream consumer + reads as "implicit feedback was applied here". The source constant + survives — it is still the exclusion key that decides which + feedback events count as explicit for cancellation. + """ s = _store(_mk("b1")) enqueue_retrieval_exposures(s, ["b1"], now=T0) sweep_deferred_feedback( s, now=T_AFTER_GRACE, grace_seconds=1800, epsilon=0.05 ) events = s.list_feedback_events(belief_id="b1") - sources = [e.source for e in events] - assert RETRIEVAL_DRIVEN_FEEDBACK_SOURCE in sources - # And NOT the same as any explicit-feedback source. - assert "user" not in sources + assert events == [] + assert RETRIEVAL_DRIVEN_FEEDBACK_SOURCE not in [e.source for e in events] # --- AC7: idempotency + crash-safe --------------------------------------- -def test_sweep_twice_equals_sweep_once() -> None: +def test_sweep_twice_reports_the_same_numbers() -> None: + """Repeatability, which is a stronger property than the idempotency + it replaces. The mutating sweeper was idempotent by consuming its + input: run twice, the second run reported zero. That reads as + "there is nothing here" rather than "this was already spent". The + audit consumes nothing, so the count is a standing measurement. + """ s = _store(_mk("b1"), _mk("b2")) enqueue_retrieval_exposures(s, ["b1", "b2"], now=T0) s.insert_feedback_event( "b2", valence=-1.0, source="user", created_at=T_BEFORE_GRACE ) - sweep_deferred_feedback( + r1 = sweep_deferred_feedback( s, now=T_AFTER_GRACE, grace_seconds=1800, epsilon=0.05 ) state_after_first = ( @@ -271,7 +308,8 @@ def test_sweep_twice_equals_sweep_once() -> None: r2 = sweep_deferred_feedback( s, now="2026-04-28T02:00:00Z", grace_seconds=1800, epsilon=0.05 ) - assert r2.applied == 0 and r2.cancelled == 0 + assert (r2.would_apply, r2.would_cancel) == (r1.would_apply, r1.would_cancel) + assert r1.would_apply == 1 and r1.would_cancel == 1 state_after_second = ( s.get_belief("b1").alpha, # type: ignore[union-attr] s.get_belief("b2").alpha, # type: ignore[union-attr] @@ -280,36 +318,159 @@ def test_sweep_twice_equals_sweep_once() -> None: assert state_after_first == state_after_second -def test_already_applied_row_is_not_reapplied() -> None: +def test_sweep_leaves_every_row_enqueued() -> None: + """The queue is not drained by being read. Pre-#1162 this row would + have flipped to `applied` and dropped out of the pending scan.""" s = _store(_mk("b1")) enqueue_retrieval_exposures(s, ["b1"], now=T0) sweep_deferred_feedback( s, now=T_AFTER_GRACE, grace_seconds=1800, epsilon=0.05 ) - # Manually re-enqueue the SAME row would be a new row; but an - # already-applied row should be skipped on re-scan because - # list_pending_deferred_feedback filters by status='enqueued'. pending = s.list_pending_deferred_feedback( cutoff_iso="2099-01-01T00:00:00Z" ) - assert pending == [] + assert [row[1] for row in pending] == ["b1"] + + +def test_limit_does_not_mislabel_eligible_rows_as_still_in_grace() -> None: + """`pending_unmet_grace` used to be `enqueued_total - len(pending)`, + which folds two disjoint populations together: rows genuinely inside + their grace window, and eligible rows that fell past `limit`. It + then prints the sum under the former's name. + + Under the mutating sweeper that was transient — a run drained its + page, the total fell, the next run saw the remainder. Audit-only + makes it permanent: nothing drains, the same page is re-reported + forever, and no number of re-runs reaches the rest. + + 12 rows eligible, 3 genuinely in grace, `limit=5`. The honest + answer is 3 in grace and 7 eligible-but-unaudited; the subtraction + form said 10 were in grace. + """ + beliefs = [_mk(f"b{i}") for i in range(15)] + s = _store(*beliefs) + enqueue_retrieval_exposures(s, [f"b{i}" for i in range(12)], now=T0) + # Enqueued "now", so their grace window has not elapsed at T_AFTER_GRACE. + enqueue_retrieval_exposures( + s, [f"b{i}" for i in range(12, 15)], now=T_AFTER_GRACE + ) + + r = sweep_deferred_feedback( + s, now=T_AFTER_GRACE, grace_seconds=1800, epsilon=0.05, limit=5 + ) + + assert r.would_apply == 5 + assert r.pending_unmet_grace == 3, "eligible rows counted as in-grace" + assert r.pending_beyond_limit == 7 + # And the three figures account for the whole queue exactly once. + assert ( + r.would_apply + r.would_cancel + + r.pending_unmet_grace + r.pending_beyond_limit + ) == 15 + + +def test_alpha_withheld_is_a_clean_report_figure() -> None: + """`would_apply * eps` is a float product: 12 * 0.05 lands on + 0.6000000000000001. Rounded at the assignment because this is a + number printed to an operator.""" + beliefs = [_mk(f"b{i}") for i in range(12)] + s = _store(*beliefs) + enqueue_retrieval_exposures(s, [b.id for b in beliefs], now=T0) + + r = sweep_deferred_feedback( + s, now=T_AFTER_GRACE, grace_seconds=1800, epsilon=0.05 + ) + + assert r.would_apply == 12 + assert r.alpha_withheld == 0.6 + + +def test_audited_row_ids_are_exactly_the_classified_rows() -> None: + """What `--gc` is allowed to delete. Bounded by `limit` in the same + way the report is, so the two cannot describe different sets.""" + beliefs = [_mk(f"b{i}") for i in range(8)] + s = _store(*beliefs) + enqueue_retrieval_exposures(s, [b.id for b in beliefs], now=T0) + + r = sweep_deferred_feedback( + s, now=T_AFTER_GRACE, grace_seconds=1800, epsilon=0.05, limit=3 + ) + + assert len(r.audited_row_ids) == 3 + assert len(r.would_apply_belief_ids) == 3 + pending = s.list_pending_deferred_feedback( + cutoff_iso="2099-01-01T00:00:00Z", limit=3 + ) + assert r.audited_row_ids == [row[0] for row in pending] + + +def test_purge_survives_an_id_list_past_the_sqlite_bind_cap() -> None: + """`--limit` is user-settable and the sweeper's own guidance is to + raise it, so the purge has to survive an id list longer than + SQLite's `SQLITE_LIMIT_VARIABLE_NUMBER` (32,766 on this connection). + A single `IN (...)` with one bind per id raises `too many SQL + variables` at 32,767 — turning "widen the run to collect the + backlog" into an opaque failure, and `_cmd_sweep_feedback` returns + from inside its `try`, so the audit block never prints either. + + Sized by the *bind count*, not the row count: the statement binds + every id whether or not a row matches, so 40k mostly-absent ids + reproduce it exactly while the store stays small enough to build in + milliseconds. Getting this wrong is easy — an earlier version of + this test used ~800 ids on the theory that crossing a chunk + boundary was enough, and passed against the unchunked code. + """ + s = _store(_mk("b1"), _mk("b2")) + enqueue_retrieval_exposures(s, ["b1", "b2"], now=T0) + real = [ + row[0] for row in s.list_pending_deferred_feedback( + cutoff_iso="2099-01-01T00:00:00Z" + ) + ] + assert len(real) == 2 + # Well past the 32,766 cap; the absent ids still occupy bind slots. + padded = real + list(range(10_000_000, 10_040_000)) + + removed = s.purge_enqueued_deferred_feedback(padded) + + assert removed == 2 + assert s.count_deferred_feedback_by_status() == {} + + +def test_purge_leaves_the_swept_audit_trail_alone() -> None: + """The `status = 'enqueued'` guard, checked rather than trusted: an + id list must not be able to take an `applied` or `cancelled` row + even when it names one.""" + s = _store(_mk("b1"), _mk("b2")) + enqueue_retrieval_exposures(s, ["b1", "b2"], now=T0) + rows = s.list_pending_deferred_feedback(cutoff_iso="2099-01-01T00:00:00Z") + s._conn.execute( # noqa: SLF001 - fixture writes the trail directly + "UPDATE deferred_feedback_queue SET status='cancelled' WHERE id=?", + (rows[1][0],), + ) + s._conn.commit() # noqa: SLF001 + + removed = s.purge_enqueued_deferred_feedback([r[0] for r in rows]) + assert removed == 1 + assert s.count_deferred_feedback_by_status() == {"cancelled": 1} -def test_partial_progress_resumes_correctly() -> None: - """Three rows; sweep one, then sweep again — the remaining two land.""" + +def test_limit_bounds_the_audit_without_consuming_the_rest() -> None: + """`--limit` still bounds one pass, but since nothing is consumed a + subsequent unbounded pass sees all three rather than the remainder.""" s = _store(_mk("b1"), _mk("b2"), _mk("b3")) enqueue_retrieval_exposures(s, ["b1", "b2", "b3"], now=T0) r1 = sweep_deferred_feedback( s, now=T_AFTER_GRACE, grace_seconds=1800, epsilon=0.05, limit=1 ) - assert r1.applied == 1 + assert r1.would_apply == 1 r2 = sweep_deferred_feedback( s, now=T_AFTER_GRACE, grace_seconds=1800, epsilon=0.05 ) - assert r2.applied == 2 - # All three got exactly one increment. + assert r2.would_apply == 3 assert all( - s.get_belief(b).alpha == 1.05 # type: ignore[union-attr] + s.get_belief(b).alpha == 1.0 # type: ignore[union-attr] for b in ("b1", "b2", "b3") ) @@ -362,7 +523,18 @@ def test_resolve_epsilon_toml_override(tmp_path: Path) -> None: assert resolve_epsilon(start=tmp_path) == 0.10 -def test_is_enqueue_on_retrieve_default_true() -> None: +def test_is_enqueue_on_retrieve_default_off() -> None: + """#1162. Default-on wrote a queue row per surfaced belief on every + `retrieve()`, on the argument that the queue is additive — true only + while nothing schedules the sweeper. It is also a second route to + the posterior bump #1086 turned off. Opt-in now.""" + assert is_enqueue_on_retrieve_enabled() is False + + +def test_is_enqueue_on_retrieve_env_on(monkeypatch) -> None: + """The opt-in is still reachable, so the default is a default and + not a removal.""" + monkeypatch.setenv("AELFRICE_IMPLICIT_FEEDBACK_ENQUEUE", "1") assert is_enqueue_on_retrieve_enabled() is True @@ -374,20 +546,19 @@ def test_is_enqueue_on_retrieve_env_off(monkeypatch) -> None: # --- Integration: epsilon respected end-to-end -------------------------- -def test_custom_epsilon_landed_in_alpha_and_audit() -> None: +def test_custom_epsilon_is_reported_as_withheld_not_applied() -> None: + """epsilon still resolves and still sizes the projection — it just + reaches `alpha_withheld` instead of `alpha`.""" s = _store(_mk("b1")) enqueue_retrieval_exposures(s, ["b1"], now=T0) - sweep_deferred_feedback( + r = sweep_deferred_feedback( s, now=T_AFTER_GRACE, grace_seconds=1800, epsilon=0.25 ) + assert r.epsilon_used == 0.25 + assert r.alpha_withheld == pytest.approx(0.25) b = s.get_belief("b1") - assert b is not None and b.alpha == 1.25 - events = s.list_feedback_events(belief_id="b1") - retrieval_events = [ - e for e in events if e.source == RETRIEVAL_DRIVEN_FEEDBACK_SOURCE - ] - assert len(retrieval_events) == 1 - assert retrieval_events[0].valence == 0.25 + assert b is not None and b.alpha == 1.0 + assert s.list_feedback_events(belief_id="b1") == [] def test_propagate_off_locked_neighbours_unchanged() -> None: @@ -439,16 +610,13 @@ def test_sweep_does_not_bump_a_locked_belief() -> None: result = sweep_deferred_feedback(s, now=T_AFTER_GRACE) - assert result.applied == 0 - assert result.skipped_locked == 1 - assert result.cancelled == 1 + assert result.would_apply == 0 + assert result.would_skip_locked == 1 + assert result.would_cancel == 1 after = s.get_belief("b1") assert after is not None assert after.alpha == 9.0 assert after.beta == 0.5 - # Row drained, not left to accumulate against a future unlock. - assert s.count_deferred_feedback_by_status().get("enqueued", 0) == 0 - # And no audit row claims an application that never happened. assert s.list_feedback_events(belief_id="b1") == [] @@ -460,11 +628,11 @@ def test_sweep_still_bumps_an_unlocked_belief() -> None: result = sweep_deferred_feedback(s, now=T_AFTER_GRACE) - assert result.applied == 1 - assert result.skipped_locked == 0 + assert result.would_apply == 1 + assert result.would_skip_locked == 0 after = s.get_belief("b1") assert after is not None - assert after.alpha > 1.0 + assert after.alpha == 1.0 def test_sweep_does_not_bump_a_foreign_belief(monkeypatch) -> None: @@ -487,45 +655,50 @@ def _foreign(belief_id: str) -> None: result = sweep_deferred_feedback(s, now=T_AFTER_GRACE) - assert result.applied == 0 - assert result.skipped_foreign == 1 - assert result.cancelled == 1 + assert result.would_apply == 0 + assert result.would_skip_foreign == 1 + assert result.would_cancel == 1 after = s.get_belief("b1") assert after is not None assert after.alpha == 1.0 - assert s.count_deferred_feedback_by_status().get("enqueued", 0) == 0 assert s.list_feedback_events(belief_id="b1") == [] -def test_sweep_eligibility_checks_share_the_row_transaction() -> None: - """Hypothesis: the lock/ownership checks read inside the row's write - transaction, not before it. +def test_sweep_issues_no_write_statement_at_all() -> None: + """Successor to #1168's check-then-act ordering test. + + That test pinned `BEGIN IMMEDIATE` before the eligibility read, so + a lock committed mid-row could not land +epsilon on a belief the + checks had just rejected. #1162 closes that window by removing the + write rather than ordering it, so the structural assertion becomes + the stronger one: the sweep must issue no INSERT, UPDATE, DELETE or + write transaction whatsoever. - Otherwise a lock committed between the check and the alpha write lands - on a belief the checks just rejected. Asserting on the statement order - pins the structure: `BEGIN IMMEDIATE` must precede the belief read. - Falsifiable by a belief SELECT appearing before the BEGIN.""" + Statement-level rather than state-level on purpose — asserting that + alpha did not move would still pass for a sweep that wrote and then + happened to write the same value, or that mutated some other table. + """ s = _store(_mk("b1", "apple banana")) - enqueue_retrieval_exposures(s, ["b1"], now=T0) + locked = _mk("b2", "apple cherry") + locked.lock_level = LOCK_USER + s.insert_belief(locked) + enqueue_retrieval_exposures(s, ["b1", "b2"], now=T0) statements: list[str] = [] s._conn.set_trace_callback(statements.append) try: - sweep_deferred_feedback(s, now=T_AFTER_GRACE) + result = sweep_deferred_feedback(s, now=T_AFTER_GRACE) finally: s._conn.set_trace_callback(None) - begins = [ - i for i, stmt in enumerate(statements) - if "begin immediate" in stmt.lower() + writes = [ + stmt for stmt in statements + if stmt.strip().lower().startswith( + ("insert", "update", "delete", "begin immediate", "begin ") + ) ] - belief_reads = [ - i for i, stmt in enumerate(statements) - if "from beliefs" in stmt.lower() and "select" in stmt.lower() - ] - assert begins, f"no BEGIN IMMEDIATE issued: {statements}" - assert belief_reads, f"no belief read issued: {statements}" - assert begins[0] < belief_reads[0], ( - "eligibility read happens before the write lock is taken: " - f"{statements}" - ) + assert writes == [], f"audit-only sweep issued writes: {writes}" + # The sweep really did traverse the rows it declined to write to — + # a no-op that read nothing would also issue no writes. + assert result.would_apply == 1 + assert result.would_skip_locked == 1 diff --git a/tests/test_implicit_feedback_age_correlation.py b/tests/test_implicit_feedback_age_correlation.py index 1311601e4..05fc27916 100644 --- a/tests/test_implicit_feedback_age_correlation.py +++ b/tests/test_implicit_feedback_age_correlation.py @@ -4,6 +4,12 @@ (i.e., older beliefs do not accumulate disproportionate alpha bumps purely because they are older, independent of retrieval frequency). +**Since #1162 the sweeper is audit-only and produces no drift at all**, so +the primary assertion is now exact zero rather than a calibrated bound. The +correlation machinery below is retained rather than deleted: it re-arms +automatically if drift ever becomes non-constant again, which is precisely +the change that would need this guard. + Two correlation measures are checked because Pearson r only catches *linear* relationships — a non-linear "becomes a clock" failure mode (U-shape, plateau, threshold) would slip past it. This test uses: @@ -355,7 +361,7 @@ def test_age_alpha_correlation_below_threshold() -> None: rng = random.Random(RNG_SEED) store, belief_ids, sweep_at = _build_workload(rng) - sweep_deferred_feedback( + result = sweep_deferred_feedback( store, now=_fmt(sweep_at), grace_seconds=T_GRACE, @@ -371,6 +377,27 @@ def test_age_alpha_correlation_below_threshold() -> None: ages.append(_age_days(belief.created_at, sweep_at)) drifts.append(belief.alpha - ALPHA_INITIAL) + # #1162. The sweeper is audit-only, so the drift it can produce is + # exactly zero — a stronger statement than any correlation bound, + # and the one worth asserting first. The negative control matters: + # a workload that enqueued nothing would also show zero drift. + assert result.would_apply > 0, ( + "workload produced no eligible rows; the guard below is vacuous" + ) + assert drifts == [0.0] * len(drifts), ( + "the audit-only sweeper moved alpha on " + f"{sum(1 for d in drifts if d)} of {len(drifts)} beliefs" + ) + + if not any(drifts): + # Constant drift makes both coefficients degenerate rather than + # informative (xi on a constant y reads 1.0). The guard below is + # kept, not deleted: whoever re-wires implicit feedback into the + # posterior gets the original #555 clock check back automatically + # the moment drift stops being constant, instead of finding a + # deleted test and a calibration table nobody re-derives. + return + xi = _chatterjee_xi(ages, drifts) dcor = _distance_correlation(ages, drifts)