Skip to content

adapters/remote: min_score is inert on Cognee, and three §A4 fixes are pinned by no discriminating test #72

Description

@oxoxDev

Follow-ups from #68. The five issues raised in review all landed and are correct in behaviour — verified empirically on main @ c2b9319. These are the gaps left behind. Nothing here is a crash or a data-loss path; the largest is a recall-contract deviation on one adapter.

1. min_score is silently inert on Cognee (contract deviation)

adapters/remote/src/cognee.rs:449 declares scores_recall() -> false, and adapters/remote/src/common.rs:699 then skips the min_score filter entirely for such a dialect. Measured against a double returning score: 0.8:

min_score=None -> 1 hit (score=0.8)
min_score=0.1  -> 1 hit (score=0.8)
min_score=0.9  -> 1 hit (score=0.8)   <- below the threshold, still returned

api/src/recall.rs:62 specifies min_score as "Drop hits scoring below this threshold." A caller asking for ≥0.9 gets a hit the adapter itself decoded as 0.8.

The flag's doc justifies itself as "context-only recall carries no score field at all" — but cognee.rs:405 and cognee.rs:413 parse score off that same response, and the in-tree double supplies it. The declaration and the decode disagree.

Suggested direction: decide per-hit rather than per-dialect — filter when a hit carries a score, keep it when it doesn't. That preserves the property #68 was protecting (a scoreless backend doesn't drop 100% of results) without discarding scores the adapter already has, and scores_recall() becomes unnecessary.

2. The Cognee min_score test cannot fail

adapters/remote/src/cognee_test.rs:216-229 asserts min_score: Some(0.5) against a double whose recall returns score: 0.8. The hit clears that threshold, so the assertion holds identically whether the filter runs or not. Verified by flipping scores_recall() to true: suite stays green, 39 passed / 0 failed.

Fix: assert with a threshold above the double's score, so the test states which semantic is intended.

3. The Supermemory dual-spelling decode is pinned by nothing

adapters/remote/src/supermemory.rs:234-237 reads "similarity" and falls back to "score". That fallback is the whole of the #68 min_score fix. Deleting it reds no test (39 passed / 0 failed), because the adapter's own double emits "similarity" (supermemory_test.rs:95), the conformance double emits "score" (conformance_test.rs:323), and grep -rn min_score conformance/src/ returns nothing — the suite never exercises min_score at all.

Fix: a min_score case driven through the "score"-emitting shape, or a min_score assertion in the conformance suite so both doubles cover it.

4. The conformance Invalid arm is still never exercised

conformance/src/suite/mod.rs:631 requires a store refusal to be MemoryError::Invalid. All three in-tree doubles accept every content shape including empty, so that arm never runs — green because it is never reached. The mapping itself is correct as of #68 (common.rs:319, 400 | 422 => Invalid) and is directly tested in failure_test::a_400_refusal_is_invalid_not_backend; this is purely about the suite arm.

Fix: give one double a validation refusal (400 on empty content) so the arm executes.

5. A retrying read can block ~181s with no signal

common.rs:363-381: up to 3 attempts, each under the 60s default deadline, plus 250ms + 500ms backoff → 180.75s worst case. No counter, log line, or metric — adapters/remote/Cargo.toml has no tracing/log/metrics dependency. #68 added a doc note (common.rs:202-205) but no observability, so a host seeing a three-minute stall cannot attribute it to retries.

Fix: emit the attempt count on the final error, or add the dependency and log each retry.

6. Health-reason redaction is coupled to prose with nothing enforcing it

common.rs:806-822 (health_reason) keeps a message's head and drops everything after " — ", which is where status_error (common.rs:288) puts the backend's response body. This works today and is tested. But the invariant — every message that interpolates a backend body must place it after that separator — lives only in a comment. A future message built without it would carry the body to the standing status surface silently.

Fix: build the redactable detail through one constructor so the separator can't be omitted, or add a test that fails if status_error's format changes shape.

7. Smaller items

  • Attempts::Once (common.rs:141-143) carries #[allow(dead_code)] and has zero call sites — all 13 are RetryTransient. The write helper empty (common.rs:391) doesn't take the marker at all, so the read/write split is held by the counter test rather than by the type. Threading Attempts through empty would make the guarantee structural.
  • with_request_timeout is triplicated verbatim: cognee.rs:32, supermemory.rs:36, mem0.rs:80.

Verified as correct — no action

Re-checked empirically on main @ c2b9319 and confirmed fixed: deep health reaches all three public types (401 → Down, 503 → Degraded); the read/write retry split (3 vs 1 attempts, mutation-tested); 400/422Invalid on all three adapters; the vendor/tinycortex gitlink restored to 8401346 (an ancestor of tinycortex@main); credentials redacted from the health reason; Mem0's both-probes-failed context reaching reason. Gates green: 24 suites, 1450 passed / 0 failed / 3 ignored.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions