fix(memory+guard): quarantine scan-rejected atoms and fix socket_path transport - #87
Merged
Merged
Conversation
… transport Two follow-ups to the PIGuard false-positive investigation (#85): 1. Scan-rejected atoms no longer vanish. The guard scan in addAtom ran before quarantine routing and hard-dropped rejections, so any guard false positive silently destroyed a legitimate memory with no review path. Rejections now go to quarantine with a recorded reason ("scan_rejected: ..." alongside "tainted"), visible via odek memory extended quarantine. PromoteAtom skips the guard rescan: the human review is the approval, and a rescan would reject the same false positive again. The extractor no longer pre-scans atoms - the single scan gate is at persistence, so extraction and manual adds get identical quarantine-on-reject behavior. 2. guard.socket_path actually works now. The piguard client dialed the Unix socket but spoke HTTP, while the go-prompt-injection-guard daemon speaks newline-delimited JSON - every scan errored and fallback_to_local silently degraded to local-only scanning. Socket mode now forwards the request as one NDJSON line per call (matching the daemon protocol), surfaces {"error":...} daemon replies, and is covered by fake-daemon socket tests plus an E2E run against the real daemon container (BENIGN facts pass, injections rejected, batch/long work).
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
odek | 06bfda4 | Commit Preview URL Branch Preview URL |
Jul 22 2026, 08:21 AM |
jkyberneees
added a commit
that referenced
this pull request
Jul 22, 2026
…y memory (#88) Consolidates fixes for 21 audited bugs in the memory system. Correctness: - extended: scan-rejected atoms now go through enforceCap before quarantine (regression from #87: quarantine could exceed max_size_mb and wedge the store, since the evictor only evicts trusted atoms) - extended: ReturnAfterBreak summarized the 5 OLDEST atoms (backwards iteration over a newest-first list) instead of the most recent - extended: final recall union was re-sorted by pure RetentionScore, discarding the blended 0.6*similarity+0.4*retention score and the LLM rerank order; the union is now deduped by ID and sorted by composite - extended: extracted atoms dedup by normalized text (refresh CreatedAt, keep higher confidence) instead of accumulating duplicates - extended: eviction now removes association links (no dangling IDs), ForgetAtom works for quarantine-only atoms, UTF-8-safe truncation, quarantine TTL eviction no longer writes under a read lock - loop: skill/episode recall no longer re-runs on every iteration when the result is empty (dedup key was only set on non-empty results) - loop: per-message dedup keys reset between Run/RunWithMessages calls (a repeated identical REPL message previously skipped all memory hooks) - legacy: merge_on_write no longer drops facts on 30-char prefix collisions (new index-based FactStore.ReplaceAt) - legacy: episode Search query cache invalidated on write/promote/prune - legacy: BuildSystemPrompt includes the extended user-state block even when legacy facts are empty, and caches the empty result - legacy: LLM ranker's explicit 'none relevant' is honored instead of being overridden with unranked candidates - legacy: Consolidate enforces the facts char cap and actually trims - extended: fenced/preamble-wrapped LLM JSON (extractor, user-model inference, predictor) now parses instead of silently dropping output Efficiency: - extended: batch per-turn atom adds - one index rebuild per turn instead of one per atom, one assoc.Persist per batch - extended: vector index reuses one embedder across rebuilds so the HTTP embedding cache actually warms (was a full-corpus re-embed over the network per added atom); same sharing for episode dedup + index via embedding.Shared (HTTP backend only; RP stays per-corpus) - extended: recall fan-out cut from ~8 LLM calls to 2 (type filtering over the existing candidate set, no rerank for predicted intents) - extended: one store load per recall query instead of a full atoms.json re-parse per candidate; AnaphoraResolve drops its duplicate search - extended: per-turn extraction + anaphora run async via MemoryManager.RunBackground instead of blocking the ReAct loop - legacy: session-end episode extraction/consolidation are tracked and drained with a bounded 15s wait in Agent.Close, so episodes survive CLI exit instead of dying with the process - extended: background user-model inference guarded against overlap and pending-review duplicates Tests: 30+ new tests across memory/extended/loop/embedding/odek; full suite, -race on memory+loop+embedding, and repo-wide golangci-lint all clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-ups to the PIGuard false-positive investigation in #85. Two independent fixes for Extended Memory reliability with the guard enabled.
Fix 1 — guard-rejected atoms go to quarantine instead of vanishing
Problem:
addAtomran the injection scan before quarantine routing and returned the error, so a guard rejection (false positive or not) silently destroyed the memory. The extractor also pre-scanned and dropped atoms at extraction time. With the sidecar in the loop, any false positive meant permanent, unreviewable memory loss — and even a correct rejection gave the operator no way to inspect what was blocked.Change:
reason(scan_rejected: ..., alongsidetaintedfor untrusted source classes).odek memory extended quarantinenow prints the reason per entry.PromoteAtomskips the guard rescan — the human review IS the approval; a rescan would reject the same false positive again, making promotion of scan-rejected atoms impossible.addAtom), so per-turn extraction, tool adds, and programmatic adds all get identical quarantine-on-reject behavior.Policy change to be aware of: previously a tainted atom matching an injection pattern was never stored anywhere (
TestQuarantineScanBeforeStore); now it lands in quarantine (never in recall) pending human review. This is the intended trade-off — quarantine content is never recalled or injected into prompts; it only exists for CLI review and ages out viaquarantine_ttl_days.Fix 2 —
guard.socket_pathspeaks the daemon's actual protocolProblem: the piguard client dialed the configured Unix socket but sent HTTP POSTs, while the go-prompt-injection-guard daemon speaks newline-delimited JSON (one JSON object per line). Every scan errored (
malformed HTTP response), and withfallback_to_local: true(the default) this silently degraded to local-only scanning — operators who configuredsocket_pathhad no semantic guard while believing they did.Change: socket mode now forwards each request as one NDJSON line per call (same framing the vendored HTTP gateway uses), shares the response handling with HTTP mode via a common
rpcpath, and surfaces{"error": ...}daemon replies as errors instead of decoding them as empty classifications.docs/CONFIG.mdupdated (socket_pathsemantics, and thethresholdrow now documents the label-confidence semantics fixed in #85).Verification
TestScanRejectedAtomQuarantined(reason recorded, excluded from live store, promote restores),TestPromoteBypassesGuardRescan(guard rejecting everything, promote still succeeds), extractor pass-through tests, socket-mode tests with a fake NDJSON daemon (detect/batch/long, dial error).guard.Newclient over the Unix socket — benign factsBENIGN/accepted, injectionsINJECTIONat 0.9999/rejected, batch and long endpoints working.go test ./...full suite green,-racegreen on guard+memory,golangci-lint0 issues.Stacked on top of #86 (daemon build) only for local testing; the diff is independent.