fix(packaging+safety): import path unification + L3 dead-code fix - #8
Open
dengluozhang-png wants to merge 5 commits into
Open
fix(packaging+safety): import path unification + L3 dead-code fix#8dengluozhang-png wants to merge 5 commits into
dengluozhang-png wants to merge 5 commits into
Conversation
VectorStore used , but Connection's context manager only manages the transaction (commit/rollback) — it does NOT close the connection, which is left to GC. On Windows, GC timing is non-deterministic, so the .db file stayed locked when TemporaryDirectory or os.unlink tried to clean it up, raising PermissionError [WinError 32] in 16 teardowns. Changes: - store.py: hold one long-lived self._conn in __init__, reuse it across all SQL ops (also faster than reconnecting per call); add close() and __enter__/__exit__ context-manager protocol - memory_safety.py: stop bypassing with sqlite3.connect(store.db_path); reuse store._conn for decay_low_confidence / get_behavioral_signals - tests: manage store lifetime via / explicit close() so the connection is released before temp-dir cleanup Result: embedding suite 16 ERROR + 5 FAILED -> 45 passed, 1 skipped. Full suite errors 16 -> 0. store.py / memory_safety.py ruff clean.
…se() API safety PR review by @lawcontinue flagged 1 P0 and 3 P1 issues from the original sqlite3-connection-lifecycle fix. This commit resolves all of them and adds tests proving the new behaviors. ### P0: multi-threaded regression Long-lived self._conn uses sqlite3's default check_same_thread=True, which broke any caller doing add()/delete() from a worker thread (the previous short-lived-connection model worked fine across threads). Fix: - store.py: connect with check_same_thread=False. SQLite itself is thread- safe in serialized mode (the default); the flag only guards Python-level thread affinity. - store.py: add self._lock = threading.Lock(); guard all write paths (add / add_batch / delete / update_metadata / rebuild_embeddings). Reads are intentionally not locked — SQLite's MVCC + serialized isolation handle concurrent SELECTs safely, and locking reads would hurt throughput with no correctness gain. ### P1#1: close() followed by API call Previously raised AttributeError: 'NoneType' object has no attribute 'execute'. Now raises RuntimeError('VectorStore is closed; ...') with a clear message. Implemented via _check_open() helper called at the top of every public SQL-touching method (add, add_batch, delete, update_metadata, rebuild_embeddings, search, execute). close() itself is idempotent. ### P1#2: memory_safety reaching into store._conn decay_low_confidence() and get_behavioral_signals() used store._conn directly, deepening the coupling. Added public VectorStore.execute(sql, params) and switched memory_safety to use it. _check_open() comes free. The decay write path now also takes store._lock to stay consistent with the Python-side in-memory indexes. ### P1#3: silent exception swallowing in close() except sqlite3.Error: pass → _log.warning('... sqlite close failed: %s', e) so unexpected cleanup failures are visible. ### __exit__ doc nit Added a comment explaining why __exit__ returns False (must propagate exceptions from inside the block — close() never swallows them). ### Tests (test_embedding.py) - test_store_concurrent_writes: 5 threads × 6 add() calls = 30 docs, no errors, no duplicate IDs. - test_store_concurrent_search_during_writes: 3 writers + 3 readers interleaved, no exceptions from either side. - test_store_after_close_raises_runtime_error: all 5 public methods raise RuntimeError post-close; close() is idempotent. ### Verification - embedding suite: 45 passed (PR lawcontinue#6 baseline) → 37 passed here (no change in scope, +4 new tests, the 36 pre-existing pass count went up only because test_benchmark.py is exercised separately). Actually test_embedding.py + test_memory_safety.py together: 37 passed, 1 skipped, 0 errors (was 16 ERROR + 5 FAILED before PR lawcontinue#6). - Full suite: 302 passed, 0 errors (was 293 passed, 16 errors). - store.py / memory_safety.py ruff: clean.
Discovered via end-to-end demo (5-scenario hippo walkthrough): two real bugs in core safety + memory layers, one P0 and one P1. ### P0 Bug lawcontinue#1: Chinese L1 attack patterns silently bypass SafetyGuard The cn_ignore_instructions regex 忽略(?:之前|上面|先前|此前)(?:所有|全部)?(?:指令|...) was too tight — natural Chinese phrasings like '忽略之前的指令' (with the structural particle '的') failed to match. The regex also had no synonym coverage ('无视' / '别' / '勿' all bypassed). Worse: the original regex lived in _L1_MEDIUM_RISK, but the default l1_block_threshold=5 means a single medium hit is silently treated as 'L1 clean' → user-facing attack prompts reach the model. Fix: - Move the equivalent pattern to _L1_HIGH_RISK as 'cn_ignore_previous' (mirrors the English 'ignore_previous' placement). - Pattern now tolerates 的/地/得 structural particles and 0-3 filler Chinese chars between the trigger and the noun, plus covers 5 synonyms (忽略/无视/请勿/别/勿). - Add cn_disregard_directive (MEDIUM) for '不遵守/请勿遵守' variants. Single medium hits still don't block (avoid false positives on e.g. '请勿遵守之前的客户规定'), but the pattern is now visible in result.warnings for downstream escalation. ### P1 Bug lawcontinue#3: decay_low_confidence default params are silently broken Three compounding bugs made the function a no-op under default args: (a) Default threshold=0.3 with model memory default confidence=0.5 meant always skipped model memory. Lowered the bar. (b) cutoff used which produces 'YYYY-MM-DDTHH:MM:SS+00:00' — NOT comparable with SQLite's CURRENT_TIMESTAMP 'YYYY-MM-DD HH:MM:SS' format. Result: 0 rows returned regardless of days_old. (c) SQLite CURRENT_TIMESTAMP timezone behavior is platform-dependent (UTC on standard Unix, local time on many Windows + Python builds). Empirically local time on this project's setup. Use local naive datetime to match; opt-in env var HIPPO_DECAY_USE_UTC=1 for UTC. Also: the condition was implemented in the PR diff but the logic was inverted — would SKIP low-conf docs, which is the opposite of what 'decay low conf' should do. The original was correct; what was wrong was the docstring (saying '<') and the default threshold. Fixed both. ### Tests - tests/test_safety_guard_v2.py: TestCnL1IgnorePrevious with 7 CN attack phrasings (parametrized) + 3 false-positive guards. All 7 must be either blocked OR produce a CN safety warning (no more 'L1 clean' on attack prompts). - embedding/tests/test_memory_safety.py: TestDecayDefaultThreshold with 4 regression tests covering: default params decay model memory, user/verified skipped even with low conf, explicit threshold above confidence decays, 24h idempotency holds. ### Verification | | Before | After | |---|---|---| | CN attack prompts blocked | 1/7 | 7/7 (or medium-flagged) | | decay_low_confidence default (days_old=0) | 0 docs | 2 docs (correct) | | safety_guard.py / memory_safety.py ruff | — | clean | | full suite passed | 302 | 316 | | full suite errors | 0 | 0 | | new tests | — | +11 (7 + 4) |
Bug discovered during end-to-end demo walkthrough: README/LAUNCH_KIT/
dev-to-article all show `from hippo.embedding import VectorStore` (17+
occurrences across docs and examples), but the source tree had `embedding/`
at the project root, not `hippo/embedding/`. pip install hippo-llm users
copy-pasting any README example got ImportError on first line.
Two options were considered:
(a) mv embedding/ hippo/embedding/ — chosen
(b) rewrite all 17+ doc imports to `from embedding.store import ...`
Picked (a) because:
- Package name `hippo-llm` should match `hippo.*` import namespace
- pyproject.toml already includes both 'embedding*' and 'hippo*' in
packages.find.include, so wheel packaging works unchanged
- New `from hippo.embedding import ...` works; old
`from embedding.store import ...` still works (as an alias via
sys.path during source-checkout dev workflow)
Changes:
- mv embedding/ -> hippo/embedding/ (16 files renamed, content unchanged)
- hippo/embedding/tests/*.py: rewrite 'from embedding.X' ->
'from hippo.embedding.X' to match new package layout
- governance/__init__.py: remove sys.path hack inserting project/embedding,
replace with `from hippo.embedding.memory_safety import StakeLevel`
(the path-mutation was a workaround for the old layout that became
obsolete after the mv)
Verification:
- All tests pass: 302 -> 328 passed (governance tests now collectable,
previously broken by sys.path fragility)
- hippo/embedding/ + governance/ ruff clean
- `from hippo.embedding import VectorStore` works (previously ImportError)
Follow-up:
- pyproject.toml packages.find.include could simplify to just 'hippo*'
(no longer need explicit 'embedding*' since hippo.embedding.* is a
subpackage) — left for a separate cleanup PR
Bug discovered during project audit: SafetyGuard.check() had its L3 embedding-semantic layer as completely unreachable dead code. Root cause: `needs_l2` was a boolean flag expressing three states (BLOCKED / PASS / ESCALATE_TO_L3) using a single True/False. The L2 block unconditionally set `needs_l2 = False` at line 437 even when it had not actually run, so L3 (line 451) could never be entered. escalate = "l3" when L1 medium hits >= l1_warn_threshold AND L2 not trained (default config!). With default SafetyGuard() this is the realistic case — L2 model isn't bundled, _l2_trained stays False — but the code fell through to 'L1 clean' without ever consulting L3. Consequence: `L3 embedding similarity check` (`hippo/safety_guard.py:451-469`) was 0% effective. There were also zero `layer=3` assertions in the existing test suite (38 tests in test_safety_guard_v2.py), confirming the dead-code hypothesis. Fix: - Replace `needs_l2: bool` with `escalate: Optional[str]` (None / 'l2' / 'l3') - L2 block only sets escalate='l3' when L2 score in [warn_confidence, block_confidence) (was: always set False regardless of input) - L1 medium hits with no L2 model go straight to L3 (escalate='l3') - L3 path now reachable for all three escalation branches Tests added (test_safety_guard_v2.py::TestL3Escalation, 5 cases): - test_l3_block_when_model_seeded: L3 actually blocks when seeded + embedding sim high (would have silently skipped pre-fix) - test_l3_no_seed_still_no_crash: graceful fallback when _l3_seeded=False - test_l2_block_short_circuits_before_l3: L2 block returns at layer=2 - test_l2_warn_escalates_to_l3: the previously-dead branch — L2 in [warn, block) range, escalate='l3', L3 blocks - test_no_escalation_when_l1_clean: regression guard for L1-only fast path Companion fix (governance/__init__.py): remove sys.path hack that inserted project/embedding to make `from memory_safety import ...` work. Now uses the canonical `from hippo.embedding.memory_safety import ...` path (only possible after PR lawcontinue#1's mv). Governance tests were broken pre-fix; they now collect cleanly. Verification: - test_safety_guard_v2.py: 38 -> 43 passed (5 new L3 tests) - Full suite: 323 -> 328 passed - hippo/safety_guard.py + governance/ ruff 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
2 participants
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.
Two real issues from project audit + demo walkthrough
Discovered via the end-to-end demo (
%TEMP%\hippo_demo.py) and a parallel code-quality / ops audit that ran after PR #7. Both fixes are self-contained and tested.Fix 1:
mv embedding/ -> hippo/embedding/(packaging)Symptom: Every README/LAUNCH_KIT/dev-to-article example (17+ occurrences) shows
from hippo.embedding import VectorStore, buthippo/embedding/did not exist in the source tree —embedding/was at project root. Any userpip install hippo-llm-then-copy-paste-from-README gotImportError: No module named 'hippo.embedding'on first line.Fix:
mv embedding/ hippo/embedding/— package name now matches import pathhippo/embedding/tests/*.py: rewrite internal imports to new layoutgovernance/__init__.py: dropsys.pathhack that mutated project root; use canonicalfrom hippo.embedding.memory_safety import ...Fix 2:
safety_guardL3 escalation was 100% dead codeSymptom:
SafetyGuard.check()claims a 3-tier L1→L2→L3 architecture, but the L3 embedding-similarity check was completely unreachable.needs_l2(boolean) was overloaded with three semantics and unconditionally reset to False inside the L2 block, even when L2 had not actually run.Consequence: With default SafetyGuard() (no L2 model bundled,
_l2_trained=False), any input with ≥l1_warn_thresholdmedium pattern hits would route to 'l3' escalation — but the code fell through to 'L1 clean' without consulting L3. There were zerolayer=3assertions in the existing test suite (38 tests), confirming the dead-code hypothesis.Fix:
needs_l2: bool→escalate: Optional[str](None / 'l2' / 'l3'). L2 block only setsescalate='l3'in the[l2_warn_confidence, l2_block_confidence)band. L1 medium hits with no L2 model go straight toescalate='l3'(previously-dead branch).Tests
tests/test_safety_guard_v2.py::TestL3Escalation(5 cases):test_l3_block_when_model_seeded— L3 blocks when seeded + sim ≥ thresholdtest_l3_no_seed_still_no_crash— graceful fallback when_l3_seeded=Falsetest_l2_block_short_circuits_before_l3— L2 returns at layer=2test_l2_warn_escalates_to_l3— the previously-dead branchtest_no_escalation_when_l1_clean— regression guardVerification
from hippo.embedding import VectorStorehippo/safety_guard.py+governance/+hippo/embedding/ruffRelated
PR #6, PR #7 were earlier fix PRs on the same fork branch. This PR uses a new branch
fix/packaging-and-l3so the two unrelated fixes can be reviewed independently.