Skip to content

refactor(config): make [embedding] and [rerank] soft dependencies - #361

Open
Kendrick-Song wants to merge 3 commits into
mainfrom
refactor/embed-soft-dependency
Open

refactor(config): make [embedding] and [rerank] soft dependencies#361
Kendrick-Song wants to merge 3 commits into
mainfrom
refactor/embed-soft-dependency

Conversation

@Kendrick-Song

@Kendrick-Song Kendrick-Song commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Summary

Make [embedding] and [rerank] soft runtime dependencies so a freshly-onboarded user can run EverOS end-to-end with only [llm] configured. Previously the server refused to start without embedding, locking out anyone who just wanted keyword-only search.

Note on history: the core work was squashed into 07a7cd5; round-4 review fixes land as follow-up commits (561b5fe, 67758ec). Four rounds of Fable 5 code review (10 + 13 + 15 + 26 = 64 findings surveyed, 59 addressed as fixes, 4 accepted-as-designed, 1 refuted). The complete audit trail lives in the commit history and the "Files changed" view.

Capability tiers

Tier Configured Enabled features
1 [llm] only KEYWORD search, add/flush, md writes, cascade sync
2 [llm] + [embedding] + VECTOR / HYBRID search, reflection, skill extraction, backfill
3 [llm] + [embedding] + [rerank] + AGENTIC search, knowledge (write + search)
  • Tier upgrades require a server restart — capability accessors cache for the process lifetime.
  • Tier downgrades are read-safe: a Tier-3 user who drops [rerank] can still read / rename / delete existing knowledge documents; only write / search endpoints return 422.

What changed

  • Component accessorscomponent/{embedding,rerank,llm,multimodal}/accessor.py are the single process-wide provider singletons. service/* never maintains parallel singletons; it consumes get_*_capability() / get_llm_client() directly. Build-time ValueError from the factory is logged as *_capability_build_failed on all four accessors (was silently swallowed on 3 of 4 prior to round-4).
  • Error mappingProviderNotConfiguredError → HTTP 422 CAPABILITY_UNAVAILABLE with everos.toml section hints (never EVEROS_* env-var strings). LanceDBMigrationError fails loud with escalating recovery guidance (restart → wipe index, in that order). LLMNotConfiguredError in search maps to None for KEYWORD degradation.
  • Nullable-vector LanceDB migration — schema v2 makes the vector column nullable so Tier-1 rows can land without embeddings. Migration is guarded by a cross-process memory_root_lock (fcntl.flock + anyio.to_thread) and runs optimize(cleanup_older_than=timedelta(0)) per table after Phase-1 backfill so the manifest chain is physically pruned (not just compacted). _TABLE_SPECS has an import-time drift assertion against BUSINESS_SCHEMAS_WITH_VECTOR.
  • Cascade — all handlers register unconditionally (Tier-3 → Tier-2/1 downgrade no longer strands DELETE); embed-requiring branches inside each handler body-guard on capability.available at execution time. Handler skip logs demoted INFO → DEBUG so Tier-1 imports don't flood the sink.
  • everos cascade backfill CLI — Phase 1 (embed missing vectors) / Phase 2 (emit synthetic events for cascaded processing) / Phase 3 (sync new skill files). Exit codes: 0/1/2/3 (server-running preflight, now checked in Phase 1 too) / 4 (COMPLETED_WITH_FAILURES — includes rows where any needed side embed is still NULL post-write) / 130 (SIGINT). OMEConfig.crash_recovery_enabled = False in backfill engines prevents stale-RUNNING rows re-enqueuing into a smaller strategy registry. Phase 2 cluster_repo.find_cluster_id_for_member is scoped by (app_id, project_id, owner_id) — same-day entry_id collisions across owners are no longer possible (entry_id is per-owner unique by design).
  • /health — reports capabilities + disabled_features per tier so ops can distinguish "boots but degraded" from "boots and full". Return type is HealthResponse (Pydantic model), so OpenAPI clients get real field shapes instead of additionalProperties: true.
  • Presentation split — memory / service / infra don't import typer / click (verified by git grep, see below). TyperPresenter + run_backfill() live in entrypoints/cli/commands/_backfill_cmd.py; NullBackfillPresenter in memory/cascade/_backfill.py for tests. nothing_to_backfill(..., *, scan_failed: bool) — colour is picked from the flag, not a substring match on the message.
  • Startup hint — unconditional count_rows(filter="vector IS NULL") sweep emits unbackfilled_memory_rows (event name + hint text pinned) when Tier-1 rows exist. ParserLifespanProvider warms the everalgo.parser import at boot so /health doesn't block on first call.
  • Knowledge upload UTF-8 short-circuit_looks_like_utf8_text() routes explicit plaintext mimes (text/plain, text/markdown, text/x-rst, text/x-markdown) and known extensions (md, txt, rst, markdown, text) straight to UTF-8 decode. text/html is deliberately excluded so HTML uploads keep going through everalgo's clean_html_for_llm (strips <script>/<style>/<nav> + 1 MiB cap). Prevents 503 «Multimodal not configured» when a Tier-3 user without [multimodal] uploads a markdown doc.
  • Upload filename hardening_safe_original_filename rejects NUL bytes and > 255-byte UTF-8 filenames up front (400) instead of letting them surface as 500 at write_bytes after the md was already written.
  • Backfill Ctrl-C at prompt — the interrupt-catch tuple now covers typer.Abort (which is typer._click.exceptions.Abort under typer 0.15+ — a distinct class from the standalone click.exceptions.Abort the vendored copy diverged from). click>=8.1 is now an explicit dependency (was only pulled in transitively via uvicorn).

Sync with main

Two origin/main merges collapsed into 07a7cd5:

  1. 6dcd3eb (v1.1.4 → v1.2.0) — adds OTel tracing, /api/v2 alias, TracingLifespanProvider, per-cascade-embedding span fix, memory-op instrumentation.
  2. 42629df (PR fix: backfill missing 1.1.4 fixes into main #366) — v1.1.4 CWE-22 knowledge path traversal fix + cascade retry-budget rework + cascade/errors.pycore.errors.ExternalServiceError.

Merge decisions worth flagging for review:

  • service/search.py adopts single wrap site — component.llm accessor already applies UsageRecordingClient when observability is on; service layer never keeps a parallel LLM singleton (Round-1 CR rule: "service layer never maintains parallel singletons").
  • Knowledge router prefix — main established the /knowledge prefix + create_app() mounting under /api/v1 and /api/v2; this PR keeps main's design (docstring on _require_knowledge_capabilities rewritten to describe the current gate location — the earlier build_handlers gate was removed there per Round-2 finding B3).
  • Cascade retry classification uses ExternalServiceError from core.errors (cascade/errors.py deleted). _MAX_TOTAL_RETRIES=12 cross-cycle budget preserved.
  • MemoryRoot.default() → resolve() — this branch renamed the MemoryRoot classmethod for clearer semantics (the old name was ambiguous with "default location"). MemoryRoot is a publicly-exported class, so a default() backward-compatibility alias is kept in this PR, forwarding to resolve() with a DeprecationWarning. Removal is left to a future major release. CHANGELOG.md under Unreleased carries the migration note. The rename is orthogonal to soft-dependency work but touches 26 files; reviewers wanting to isolate its impact can start from git log --oneline --diff-filter=M -- src/everos/core/persistence/memory_root.py in the squashed commit.

Layering verification

$ git grep -l "^import typer\|^from typer" src/everos/{memory,service,infra}
# empty
$ git grep -l "^import click\|^from click" src/everos/{memory,service,infra}
# empty

memory / service / infra layers are clean of CLI presentation libraries. This is a grep-verified invariant, not an import-linter contract — memory-layer forbidden-import contracts still only cover infra.persistence.** internals; extending the contract to typer / click is a followup.

Test plan

  • make lint — ruff + import-linter (3 contracts) + assets + deprecated-names + github-docs + datetime + OpenAPI drift
  • Hermetic env EVEROS_ROOT=/tmp/… EVEROS_EMBEDDING__API_KEY="" EVEROS_RERANK__API_KEY="" EVEROS_LLM__API_KEY="" full pytest = 2037 passed / 7 deselected (7 = slow + live_llm markers)
  • Manual e2e — 21/21 assertions across Tier 1/2/3 + downgrade + v1/v2 double-mount
  • /health reports correct capabilities + disabled_features per tier
  • Four rounds of Fable 5 (opus) code review — 59 findings closed (10 + 13 + 15 + 21). See Round-4 review outcomes below.

Round-4 review outcomes

Round 4 surfaced 26 additional findings after the squash landed. Verdict / disposition:

Fixed in 561b5fe + 67758ec (21):

Severity Finding Fix
Blocker N1 cluster_repo.find_cluster_id_for_member gained (app_id, project_id, owner_id) scoping via JOIN — entry_id is per-owner-unique, so unscoped reverse lookups silently mis-attributed same-day episodes across owners
Blocker N2 Ctrl-C at the y/N prompt now dispatches to the exit-130 path (was falling through to exit-2 with rich traceback) — typer.Abort and click.exceptions.Abort are distinct classes under typer 0.15+
Blocker N3 _looks_like_utf8_text mime whitelist tightened from startswith("text/") to an explicit set — text/html no longer bypasses everalgo's HTML sanitiser
Major J3 MemoryRoot.default() restored as deprecation alias; PR body errata corrected
Major J4 list_by_owner_after_ts(limit=…) docstring now spells out the "keeps oldest N" trap
Major J5.2 / J5.5 Presenter nothing_to_backfill gains explicit scan_failed kwarg; dead phase_header Protocol method + implementations removed
Major J6.3 3 inline ProviderNotConfiguredError imports promoted to top-level
Major J7 Rows with any needed side still NULL after write count as rows_failed — exit code 4 now honest instead of silent 0
Major J9 test_migration_cross_process.pytest_migration_lock_wiring.py; docstring scopes to "wiring only" and points at test_locking.py for real flock
Major J10 Phase 1 gained the _probe_ome_lock_available preflight Phase 2/3 already had — --phase all against a live server halts before Phase 1 burns any embed budget
Minor M1 NUL / > 255-byte filenames rejected up front (400) instead of surfacing as 500 with a half-written md
Minor M2 Backfill optimize() passes cleanup_older_than=timedelta(0) to prune older manifest versions
Minor M3 verify_business_schemas remediation walks restart → wipe (was jumping straight to rm -rf)
Minor M5 Multimodal accessor logs multimodal_llm_capability_build_failed (was silent)
Minor M7 parser_available.cache_clear() in the 2 tests that swap sys.modules['everalgo.parser']
Minor M8 6 handler cascade_handler_embed_skipped logs demoted INFO → DEBUG
Minor M10.1 Drift-assertion test now reloads _backfill with a synthetic-schema override and asserts on the actual RuntimeError
Minor M11 test_cascade_verbose_position.py subprocess calls scrub EVEROS_* from env=
Minor M14 /health return type is HealthResponse Pydantic model; OpenAPI schema regenerated
Minor M16 routes/knowledge.py:_require_knowledge_capabilities docstring rewritten to describe the current design (was still citing the removed build_handlers gate)

Refuted (1):

  • M12 — review claimed cascade_backfill_row_embed_failed doesn't exist; it does, at _backfill.py:501.

Deferred to followup (13):

  • J1 — root-cause fix for parser/_core.py:74 needs everalgo signature change to support lazy LLM factory; UTF-8 short-circuit covers the user-facing path meanwhile
  • J2 — Tier definition confirms "knowledge = Tier 3 whole" (Y2 was a design proposal, rejected as-designed)
  • J5.1 / J5.3 / J5.4 — broader presentation-split refactor (domain-layer user text, auto_yes percolation, import-linter private-name contract)
  • J6.1 / J6.2run_backfill phase-dispatch and _backfill_table refactor (both ~100+ lines of restructuring)
  • J8 — Phase 1 keyset pagination for genuine bulk migration (500k+ rows)
  • M4fcntl.flock timeout / waiting log / re-entry guards (core/persistence refactor)
  • M6 — UTF-8 codec strategy (BOM / UTF-16 / GBK fallback)
  • M9count_by_owner non-monotonicity (INTERVAL=1 short-circuits it today; latent)
  • M13--phase all multi-phase combined-outcome coverage (priority is sequential-loop-defined; test additions only)
  • M15 — PR-marker rationale comments (16 occurrences, all inert)

Known follow-ups (beyond round-4)

  • _TABLE_SCHEMA_VERSION docstring — v3+ migrations need a version dispatch table
  • extract_user_profile.py throttle-counter block — replace LanceDB count_by_owner with a SQLite memcell count
  • PDF/DOCX uploads still require [multimodal] by design; the 415/503 error text could be more targeted

🤖 Generated with Claude Code

@Kendrick-Song Kendrick-Song self-assigned this Jul 27, 2026
Kendrick-Song added a commit that referenced this pull request Jul 27, 2026
Group I (ae5543d) added body-guards to 4 strategies that read
get_embedding_capability().available at execution time. Existing
tests patched the factory / get_embedder — not the accessor. On
developer machines with ~/.everos/everos.toml carrying real keys,
the accessor's lazy build made .available=True and the tests
passed. In CI (zero secrets) and BOSS's hermetic env checks, the
guard trips and 16 tests fail.

Two-part fix:
- tests/conftest.py autouse fixtures now inject Capability
  (provider=None) instead of leaving the cache as None. Any test
  that touches an accessor sees available=False by default —
  forcing hermetic hygiene at the suite level.
- Failing tests inject a stub provider via
  monkeypatch.setattr(acc, "_capability",
  EmbeddingCapability(provider=<stub>)) so the guard passes and
  the strategy body reaches its assertion sites.

Verified: full unit + integration suite green under
EVEROS_ROOT=/tmp/probe EVEROS_EMBEDDING__API_KEY=""
EVEROS_RERANK__API_KEY="" EVEROS_LLM__API_KEY="".

Fixes blocker B1 from PR #361 review.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Kendrick-Song added a commit that referenced this pull request Jul 27, 2026
Six sites across service/memorize.py and the 4 embed-requiring
strategies claimed a runtime tier upgrade "picks up on the next
dispatch without an engine restart". That claim is false: the
capability singleton at component/embedding/accessor.py caches for
process lifetime, load_settings is functools.cache, and there is
no invalidation path in src/.

Body-guards remain — they exist for defensive degradation of
direct-emit paths (tests, future features) and to keep dispatch
clean when capability was absent at server start. Comments now
state that reason plainly and explicit that tier upgrades require
a server restart. Spec doc updated to match.

Fixes blocker B2 from PR #361 review.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Kendrick-Song added a commit that referenced this pull request Jul 27, 2026
The cascade registry gated both knowledge handlers off as an
atomic pair when embed OR rerank was unavailable. But:

- KnowledgeDocumentHandler is SQLite-only, needs no capability at
  all.
- KnowledgeTopicHandler.handle_added_or_modified already calls
  embed_or_none — writes vector=None gracefully; column is
  nullable since the embedding-soft-dependency migration.
- handle_deleted on both handlers is pure repo delete — never
  touched capability.

The gate broke the delete path: on Tier 3 -> Tier 2 downgrade,
service.delete_document rmtree's the md directory expecting
cascade to clean SQLite / LanceDB, but the worker finds no
handler for the kind and marks each row failed(retryable=False)
- leaving phantom documents visible via GET /documents forever.

Remove the gate. Search-side is gated separately at the route
level (Group C, 4f27124), so cascade doesn't need to gate writes.

Fixes blocker B3 from PR #361 review.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Kendrick-Song added a commit that referenced this pull request Jul 27, 2026
/health hardcoded `"llm": True` and compute_disabled_features skipped
the llm branch — reviewer flagged asymmetry with the other four
capabilities. Not a live bug (LLMLifespanProvider fails startup on
missing LLM credentials, so /health is unreachable in that state),
but the docstring and comment now state that reason explicitly so a
future maintainer isn't misled by the asymmetry.

If LLM ever becomes soft (like embed/rerank), swap the literal for a
real probe and add the LLM-disabled-features branch to
compute_disabled_features.

Fixes PR #361 review finding M3 (doc-only Option 3).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Kendrick-Song added a commit that referenced this pull request Jul 27, 2026
Two backfill defects flagged in PR #361 review round 2:

- M1 (Group F regression): _run_phase_skills early-returned when
  _scan_skill_source found no work, so _sync_new_skill_files never
  ran on the rerun path. Combined with Group F's on-disk SKILL.md
  probe (which skips already-md'd clusters), a Ctrl-C mid-Phase 3
  would strand the orphan SKILL.md files forever — until the user
  manually ran `everos cascade sync`.
  Fix: run sync before scan, unconditionally. sync_once is a no-op
  when there's nothing new; costs a scan-once + drain-empty round
  trip. The trailing post-drain sync is kept so skills_after still
  reflects freshly-extracted skills on the happy path.

- M2: Phase 2 clustering was documented as non-idempotent but the
  CLI docstring promised "nothing is redone from scratch" and
  _print_interrupted suggested `everos cascade backfill --phase all
  --yes`. Users following the hint after a Ctrl-C would grow
  cluster counts spuriously.
  Fix: _emit_synthetic_events filters each row through the existing
  cluster_repo.find_cluster_id_for_member helper (previously
  zero-call-sites) before emitting. Already-clustered rows are
  skipped. Phase 2 is now genuinely idempotent, and the CLI claim
  is true.

  member_type strings on the lookup path (\"episode\" / \"case\") match
  what trigger_profile_clustering / trigger_skill_clustering insert
  on the write path — verified by grep and pinned by a regression
  test.

Fixes PR #361 review findings M1 and M2.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Kendrick-Song added a commit that referenced this pull request Jul 27, 2026
…first

Round-2 review found 3 defects in extract_user_profile's Tier-1
direct path — all stemming from reading LanceDB inside a strategy
triggered by an event that already carries the data:

- M4: on a fresh Tier-1 install, cascade may not have indexed the
  first memcell into LanceDB before EpisodeExtracted triggers the
  profile strategy. list_by_owner_after_ts returned [], memcell_ids
  was empty, the < PROFILE_MIN_MEMCELLS guard early-returned, OME
  marked the run success — first-memory profile permanently lost.
  The event carries memcell_id explicitly to avoid this race
  (see events.py:40-51).
- M5: direct path selector was strictly after_ts > last_profile_ts,
  while cluster path (:139) has an `or c.id == event.cluster_id`
  fallback ensuring the current event's data is always included.
  Historical-timestamp imports would slip through the inequality
  and never be profiled.
- M6: last_profile_ts=0 (user.md deleted / never existed) → return
  every memcell for the owner → memcell_repo.find_by_ids passes N
  params to a single IN clause → SQLITE_MAX_VARIABLE_NUMBER (999
  default) blowup on large histories.

Fix: _select_via_timestamp always includes event.memcell_id; LanceDB
is a supplement. memcell_repo.find_by_ids chunks by 500 to protect
the SQL param limit. Throttle counter still reads LanceDB (stale-
but-monotonic acceptable); real fix TODOd for a followup.

Fixes PR #361 review findings M4, M5, M6.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Kendrick-Song added a commit that referenced this pull request Jul 27, 2026
_build_cluster_engine and _build_skill_engine (memory/cascade/_backfill)
share the server's ome.db jobstore path. engine.start() unconditionally
ran _run_crash_recovery, which scanned run_record for stale RUNNING
rows (from a prior server session that crashed / was killed) and
re-enqueued them into the currently-starting engine. But backfill
engines register only the Phase-2/3 subset of strategies — any
re-enqueued row for a different strategy hits StrategyRegistry.get
with an unknown name, raising and permanently marking the event
CRASHED-but-not-recovered.

Add crash_recovery_enabled bool to OMEConfig (default True to keep
the server's semantic unchanged). Backfill's cluster + skill
engines pass False. Server's stale rows stay put; the next server
restart resumes them normally, from an engine that actually knows
those strategy names.

Fixes PR #361 review finding M10.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Kendrick-Song added a commit that referenced this pull request Jul 27, 2026
Round-2 review finding M7: migrate_table_schemas guarded only by
a marker file. Server startup and concurrent `everos cascade`
invocations both call ensure_business_indexes → migrate_table_
schemas. Both processes read marker == 0, both proceed to alter,
both mutate schema simultaneously. TOCTOU window between marker
check and the actual alter_columns.

The memory_root_lock helper (core/persistence/locking.py) existed
for exactly this scenario but had zero call sites in src/. Wrap
migrate_table_schemas internally with the lock, re-check the
marker after lock acquisition — first process migrates, second
waits, wakes up, sees the marker, no-ops. Applied the same
treatment to migrate_fts_indexes: identical TOCTOU pattern,
identical fix, keeps the two sibling migrations symmetric.

Also refine LanceDBMigrationError's recovery hint: races no longer
reach the raise, so it now genuinely means alter failed. Message
now escalates: (1) restart, (2) if persists, wipe the index dir.
Previous message jumped straight to `rm -rf`, harsh for what may
be a transient filesystem hiccup.

Fixes PR #361 review finding M7.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Kendrick-Song added a commit that referenced this pull request Jul 27, 2026
Round-2 review finding M8: one malformed row (text over provider
context limit, etc.) 400s the whole embed_batch call. The batch's
32 rows all get marked failed and re-scanned in the next backfill,
where they hit the same batch layout and fail the same way — an
infinite retry loop until an operator manually finds the poison
row. Log only carried batch_size + error, no row ids.

_embed_primary_batch and _embed_subject_batch now fall back to
per-row provider.embed(...) when the batch call raises. Each
per-row failure logs `cascade_backfill_row_embed_failed` with
the row_id and a truncated text_prefix (first 100 chars) so an
operator can identify and quarantine the poison row. 31 healthy
rows advance; only the poison row stays NULL and re-enters the
next scan (which now goes nowhere unless the underlying issue
is fixed).

run_backfill returns exit code 4 (COMPLETED_WITH_FAILURES) when
any phase reports rows_failed > 0, so automation can differentiate
clean success from partial success — previous behavior always
exited 0 as long as no phase raised.

Fixes PR #361 review finding M8.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Kendrick-Song added a commit that referenced this pull request Jul 27, 2026
Round-2 review finding M9: Phase 1 does per-row LanceDB updates
(one transaction per row, one manifest version per row), and
never calls optimize(). Same failure mode as v1.1.3's FTS bug
(#336, lance-format/lance#7653): unbounded manifest growth,
disk bloat, eventual optimize() breakage. 50k rows = 50k
manifest versions with nothing compacting them.

Per-row update semantic is preserved (Batch 2b's per-row failure
isolation depends on it). Add optimize() at end of each table's
backfill loop, gated on rows_processed > 0 so no-op backlogs
don't pay the compact cost. Optimize failures are best-effort
maintenance and don't invalidate the writes.

Fixes PR #361 review finding M9.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Kendrick-Song added a commit that referenced this pull request Jul 27, 2026
Round-2 review finding M11: memory/cascade/_backfill.py imported
typer + click and held PHASES / _confirm / _print_* / _EXIT_LABELS
/ _report_emit_progress. Only file in src/everos/{memory,service,
infra} doing so — import-linter doesn't check third-party imports,
so the architecture rule was silently bypassed.

Extract:
- BackfillPresenter Protocol in memory (interface, no typer)
- NullBackfillPresenter no-op default for tests / non-CLI callers
- TyperPresenter + PHASES + _EXIT_LABELS + run_backfill orchestrator
  into new entrypoints/cli/commands/_backfill_cmd.py
- Phase runners take a presenter= kwarg for user I/O

Memory layer no longer imports typer or click. cascade.py's
backfill command switches import to the entrypoints module. Test
stubs use a NullBackfillPresenter / FakePresenter to inject a
no-op / recording implementation.

Also fix M12: test_lifespan_soft_build.py's two zero-assertion
"should NOT raise" tests get real assertions on the orchestrator
instance (isinstance, provider._orchestrator, _started state);
test_health_capabilities.py's tautology
(expected_disabled = compute_disabled_features(caps)) is replaced
with parametrized hardcoded expected sets per soft-dependency tier
— so a bug in compute_disabled_features would actually break the
test.

Fixes PR #361 review findings M11 and M12.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Kendrick-Song added a commit that referenced this pull request Jul 28, 2026
Round-3 review Minor findings:

- #1 embedding/rerank accessors silently swallowed
  factory-side ValueError (unsupported provider name, malformed
  base_url, etc.) — user got 422 "not configured" with no log
  trace of why. Add logger.warning(reason=str(exc)) so an
  operator can distinguish "not configured" from "misconfigured".

- #2 two embedding provider singletons in one process: _embedder
  and _capability.provider each ran build_embedding_provider,
  spawning two AsyncOpenAI clients and two asyncio.Semaphore
  instances — silently doubling max_concurrent. Delete
  get_embedder(); consumers go through
  get_embedding_capability().require(). One provider,
  one semaphore.

- #7 ReflectionOrchestrator.run's embedding-availability guard
  was unreachable: reflect_episodes.py:69 body-guard returns
  before construction reaches it, and the orchestrator is built
  via .require() which raises on unavailable. Third check is
  dead code — delete.

Fixes PR #361 review Minor findings #1, #2, #7.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Kendrick-Song added a commit that referenced this pull request Jul 28, 2026
…ft check

Round-3 Minor findings #3 + #4:

- #3: Group J (c11de31) added a marker file + limit(1) probe to
  make _log_unbackfilled_hint "cheap" on subsequent boots. Real
  state: vector column has no scalar index (grep of
  infra/persistence/lancedb: only FTS indexes), so limit(1) and
  count_rows both cost O(N) full scan. Clean-state boot: probe
  scans entire empty tail = same as count_rows. Dirty-state boot:
  probe hits early THEN count_rows runs anyway = twice the scan.
  last_seen_count field written but never read.
  Revert: unconditional count_rows loop. Delete marker file,
  probe function, _write_marker, MemoryRoot.unbackfilled_hint_marker
  property, and _backfill.py's _clear_unbackfilled_hint_marker.

- #4: memory/cascade/_backfill.py:_TABLE_SPECS is a hand-written
  second list of the same 6 business tables that
  BUSINESS_SCHEMAS_WITH_VECTOR (infra) already enumerates. Group
  B claimed "single public source" but only fixed the lifespans
  side. Adding a new business table to infra without also
  touching _TABLE_SPECS silently drops it from backfill.
  Fix: import-time assertion that both lists' TABLE_NAME sets
  match. RuntimeError at import time surfaces drift before any
  test even starts.

Fixes PR #361 review Minor findings #3, #4.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Kendrick-Song added a commit that referenced this pull request Jul 28, 2026
…rser warm

Round-3 Minor findings #9 + #10 + #14:

- #9: episode_repo.list_by_owner_after_ts pulled full rows
  (including two 1024-D vector arrays) but its sole caller only
  reads parent_id; also missing `deprecated_by IS NULL` from both
  list and count queries — inconsistent with the rest of the
  episode read path (search filters + reflection). Add optional
  columns projection + limit; add deprecated_by filter to both
  methods; caller updated to project only parent_id.

- #10: Q2 fixed --root's asymmetry (both group and subcommand
  positions work) but --verbose was only at the group. Users hit
  "No such option: --verbose" running
  `everos cascade backfill --verbose`. Mirror Q2's pattern:
  _apply_verbose_logging helper + each of the 4 subcommands
  accepts --verbose / -v, subcommand value overrides group.

- #14: /health called parser_available() which imports
  everalgo.parser on first call — pulls in PDF/Office deps,
  blocks the event loop for hundreds of ms. Memoize with lru_cache
  + add ParserLifespanProvider to warm the import at boot.
  Optional extra remains optional; startup is a no-op when not
  installed.

Fixes PR #361 review Minor findings #9, #10, #14.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Kendrick-Song added a commit that referenced this pull request Jul 28, 2026
Round-3 Minor findings final wrap:

- #12: task-number leaks (Task 14 / 22 / 25) in released
  source docstrings replaced with descriptions of the current
  invariant. extract_user_profile.py:11-13,103-107 rewritten:
  the Task-14 registration gate it referenced was removed by
  ae5543d (Group I replaced it with per-strategy body-guards);
  the docstring's mutual-exclusion argument now correctly
  states the applies_to + body-guard mechanism.

- #13a: test_backfill_batch_concurrency wall-clock assertion
  (span < total * 0.8) replaced with overlap assertion (p_start
  < s_end AND s_start < p_end). Tests concurrent execution
  semantic without CI-runner-latency assumption.

- #13b: test_backfill_flags.py SIGINT-to-getpid test marked
  slow to protect against CI race where 0.3s timer fires before
  asyncio.Runner installs its handler. Subprocess variant
  deliberately not shipped (both variants would be slow-gated
  anyway; the added maintenance surface would not pay for
  itself). Docstring documents the exclusion rationale.

- #5/#6/#8/#11-second: design-note docstrings at the relevant
  sites so future readers know these are accepted choices, not
  oversights. #8's rationale lives once in
  trigger_profile_clustering with pointers from the three
  sibling body-guards to avoid a four-copy paste-flood.

Fixes PR #361 review Minor findings #5, #6, #8, #11 (abort
semantic), #12, #13a, #13b.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Make [embedding] and [rerank] soft runtime dependencies so a
freshly-onboarded user can run EverOS end-to-end with only [llm]
configured. Previously the server refused to start without embedding,
locking out anyone who just wanted keyword-only search.

## Capability tiers

- Tier 1 ([llm] only)                   KEYWORD search, add/flush, md
                                        writes, cascade sync
- Tier 2 ([llm] + [embedding])          + VECTOR / HYBRID search,
                                        reflection, skill extraction,
                                        backfill
- Tier 3 ([llm] + [embedding] + rerank) + AGENTIC search, knowledge

Tier upgrades require a server restart (capability accessors cache
for the process lifetime). Tier downgrades are read-safe: a Tier-3
user who drops [rerank] can still read/rename/delete existing
knowledge documents; only write/search endpoints return 422.

## What changed

- Component accessors — component/{embedding,rerank,llm}/accessor.py
  are the single process-wide provider singletons. service/* never
  maintains parallel singletons; it consumes get_embedding_capability()
  / get_rerank_capability() / get_llm_client() directly. Build-time
  ValueError from the factory is logged as capability_build_failed
  (was silently swallowed).
- Error mapping — ProviderNotConfiguredError -> 422 with everos.toml
  section hints (never EVEROS_* env-var strings).
  LanceDBMigrationError fails loud with escalating recovery guidance
  (restart -> wipe index). LLMNotConfiguredError in search maps to
  None for KEYWORD degradation.
- Nullable-vector LanceDB migration — schema v2 makes the vector
  column nullable so Tier-1 rows can land without embeddings.
  Migration is guarded by a cross-process memory_root_lock
  (fcntl.flock + anyio.to_thread) and runs optimize() per table
  after Phase-1 backfill to reclaim manifest bloat.
- Cascade — knowledge handlers register unconditionally (Tier-3 ->
  Tier-2/1 downgrade no longer strands DELETE); embed-requiring
  strategies use body-guards that check capability.available at
  execution time. _TABLE_SPECS has an import-time drift assertion
  against BUSINESS_SCHEMAS_WITH_VECTOR.
- `everos cascade backfill` CLI — Phase-1 (embed missing vectors) /
  Phase-2 (emit synthetic events for cascaded processing) / Phase-3
  (sync new skill files). Exit codes: 0 / 1 / 2 / 3 (server running
  preflight) / 4 (COMPLETED_WITH_FAILURES — per-row failures rolled
  up) / 130 (SIGINT). OMEConfig.crash_recovery_enabled=False in
  backfill engines prevents stale-RUNNING rows re-enqueuing into a
  smaller strategy registry.
- /health — reports capabilities + disabled_features per tier so ops
  can distinguish "boots but degraded" from "boots and full".
- Presentation split — memory / service / infra never import typer /
  click. TyperPresenter Protocol + run_backfill() live in
  entrypoints/cli/commands/_backfill_cmd.py. Enforced by
  import-linter.
- Startup hint — unconditional count_rows(filter="vector IS NULL")
  sweep emits unbackfilled_memory_rows (event name + hint text
  pinned) when Tier-1 rows exist. ParserLifespanProvider warms the
  everalgo.parser import at boot so /health doesn't block on first
  call.
- Knowledge upload UTF-8 short-circuit — _looks_like_utf8_text()
  routes text/* mime and known plaintext extensions (md/txt/rst)
  straight to UTF-8 decode instead of the parser. Prevents 503
  Multimodal-not-configured when Tier 3 sans [multimodal] uploads a
  markdown doc.

## Sync history with main (2 merges collapsed into this squash)

Merged origin/main at 6dcd3eb (v1.1.4 -> v1.2.0 adds OTel tracing,
/api/v2 alias, TracingLifespanProvider, per-cascade-embedding span
fix, memory-op instrumentation) and later at 42629df (PR #366
backfills v1.1.4 CWE-22 knowledge path traversal fix + cascade
retry-budget rework + errors.py -> core.errors.ExternalServiceError).
Key merge decisions:
- service/search.py adopts single wrap site — component.llm accessor
  already applies UsageRecordingClient when observability is on;
  service layer never keeps a parallel LLM singleton (Round-1 CR
  rule: "service layer never maintains parallel singletons").
- Knowledge router prefix moved to /knowledge; create_app() mounts
  it under both /api/v1 and /api/v2.
- Cascade retry classification uses ExternalServiceError from
  core.errors (cascade/errors.py deleted). _MAX_TOTAL_RETRIES=12
  cross-cycle budget preserved.
- Fixed backport typo: MemoryRoot.default() -> MemoryRoot.resolve()
  (no .default() classmethod exists — main PR #366 shipped a broken
  call).

## Verified layering

    $ git grep -l "^import typer\|^from typer" src/everos/{memory,service,infra}
    # empty
    $ git grep -l "^import click\|^from click" src/everos/{memory,service,infra}
    # empty

Memory / service / infra layers clean of CLI presentation libraries.

## Review history

Three rounds of Fable 5 (opus) code review across the pre-squash
commit history closed 38 findings total:
- Round 1: 10 findings (fail-loud migration, backfill hardening,
  knowledge router gate scoping, SearchManager guards, profile
  throttle lift)
- Round 2: 13 findings (hermetic test env, hot-reload doc drift,
  knowledge handler registration, Phase 3 sync guarantee, Phase 2
  idempotency, profile event-first path, OMEConfig crash-recovery
  gate, cross-process migration lock, batch embed per-row fallback,
  LanceDB optimize, typer/click layer split)
- Round 3: 15 Minor cleanups (accessor unification, marker revert,
  episode query hygiene, --verbose subcommand, parser lifespan warm,
  task-number scrub, temporal-overlap test, real-SIGINT slow mark,
  4 design-note back-references)

Full per-round context lives in the PR description on GitHub.

## Test plan

- make lint (ruff + import-linter 3 contracts + assets +
  deprecated-names + github-docs + datetime + OpenAPI drift)
- Hermetic env full pytest — 2027 passed / 7 deselected (7 = slow +
  live_llm markers)
- Manual e2e across Tier 1/2/3 (21/21 assertions across v1/v2
  double-mount and Tier 3 -> Tier 2 downgrade)
- /health reports correct capabilities + disabled_features per tier

## Known follow-ups

- .superpowers/sdd/followup-http-bridge.md (gitignored) — Path A for
  spec §10's "backfill 期间 EverOS 完全可用" promise
- _TABLE_SCHEMA_VERSION docstring — v3+ migrations need a version
  dispatch table
- extract_user_profile.py throttle-counter block — replace LanceDB
  count_by_owner with a sqlite memcell count

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Kendrick-Song
Kendrick-Song force-pushed the refactor/embed-soft-dependency branch from 04af701 to 07a7cd5 Compare July 28, 2026 08:01
Kendrick-Song and others added 2 commits July 28, 2026 18:31
N1. cluster_repo.find_cluster_id_for_member was cross-owner-unsafe.
Its reverse index (member_type, member_id) alone cannot disambiguate
two owners whose entry_id happens to collide — entry_id is
deliberately only per-owner unique (see entries.py:47:
'Cross-user uniqueness is handled at the database layer via a
composite <user_id>_<entry_id> field; it is not encoded into the
EntryId string itself'). Phase 2's _scan_all_rows crosses all
owners, so on any multi-owner root, same-day seq=1 episodes under
different owners would either false-hit each other's cluster or
be silently skipped from clustering. Add required (app_id,
project_id, owner_id) keyword args + JOIN Cluster (which already
carries scope) to filter by parent scope. Prior signature had zero
production callers except two the same PR just added, so the
API break is contained. Regression test: two owners persist a
cluster each around the same entry_id, each lookup resolves to
its own owner's cluster, a third owner's lookup returns None.

N2. Ctrl-C / EOF at the y/N prompt was landing on the generic
except Exception branch (exit 2 with rich traceback) instead of
the exit-130 interrupt path. Root cause: typer 0.15+ vendored
click under typer._click, so typer.Abort and the standalone
click.exceptions.Abort are distinct classes. The interrupt-branch
catch only listed the standalone one; every existing 'abort'
test was manually raising click.exceptions.Abort so the miss
was a false-positive guard rail. Widen the catch to
(typer.Abort, click.exceptions.Abort) and declare click as a
first-class dependency (it was only pulled in via uvicorn).
Regression test: raise real typer.Abort() at the confirm step
and assert exit 130 + INTERRUPTED banner.

N3. _looks_like_utf8_text used mime.startswith('text/'), which
caught text/html as well. HTML uploads then bypassed everalgo's
_aparse_html — losing clean_html_for_llm (strips <script>/<style>
/<nav>/<iframe> + HTML comments) and the 1 MiB output cap. A
40 MiB .html with <script> bodies and <!-- prompt injection -->
comments would flow straight into the extraction LLM. Replace
with explicit allowlist {text/plain, text/markdown, text/x-rst,
text/x-markdown}; text/html and any future text/* mime now
default to the parser path. Test matrix asserts text/html →
False (was regressed as True by the earlier commit).

Hermetic env full pytest: 2033 passed / 7 deselected (+6 tests
from these regressions).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round-4 review-driven cleanup. Blocker fixes (N1/N2/N3) landed in
561b5fe. This commit closes the remaining CONFIRMED items:

Major:
- J3 MemoryRoot.default() -> resolve() was a breaking public API
  rename this branch introduced (main still has default()). Adds
  default() as a backward-compat alias forwarding to resolve()
  with DeprecationWarning; CHANGELOG entry under Unreleased.
- J4 episode_repo.list_by_owner_after_ts(limit=N) truncates in
  fragment order (== insertion order), NOT newest-first. Docstring
  now spells out the trap so a future caller passing limit for a
  'newest N' window doesn't silently get the oldest N.
- J5.2 TyperPresenter.nothing_to_backfill picked colour via
  'could not be read' in message — a domain wording change
  would silently flip yellow -> green. Signature gains explicit
  scan_failed: bool kwarg; CLI colour-picks off the flag.
- J5.5 phase_header was Protocol-declared but never dispatched
  (run_backfill calls _print_phase_header directly). Removed the
  dead Protocol method + both no-op implementations.
- J6.3 3 inline from everos.core.errors import ... inside
  Phase 1/2/3 preflights promoted to a single top-level import.
- J7 subject-side embed failure was silently exit-0 because
  rows_processed advanced whenever any side wrote. Now: a row with
  a needed side still NULL counts as rows_failed (exit 4 =
  COMPLETED_WITH_FAILURES). Gated on spec.subject_of + row.needs_*
  + row.subject_text so non-Episode tables and subject-empty rows
  don't false-positive.
- J9 test_migration_cross_process.py did NOT actually test cross-
  process (all 5 tests mock memory_root_lock). Renamed to
  test_migration_lock_wiring.py; docstring now scopes it to
  'lock-invocation wiring' and points at test_core/…/test_locking.py
  for real flock coverage.
- J10 Phase 1 lacked the server-running preflight Phase 2/3 have.
  --phase all against a live server would burn Phase 1 embed API
  calls (real cost) before Phase 2 halted with exit 3. Phase 1 now
  probes _probe_ome_lock_available first; regression test in
  test_backfill_preflight.py; upgrade_path integration patches the
  probe so its in-process 'server + backfill' scenario stays valid.

Minor:
- M1 knowledge upload with NUL byte or filename > 255 bytes UTF-8
  used to raise ValueError/OSError at write_bytes → 500 with a
  half-written md left on disk. _safe_original_filename now
  rejects both up front with InvalidInputError (→ 400).
- M2 backfill optimize() now passes cleanup_older_than=timedelta(0)
  so older manifest versions are physically pruned (previous call
  compacted fragments but left the manifest chain on disk).
- M3 verify_business_schemas remediation text used to jump straight
  to 'rm -rf ~/.everos/.index/lancedb'; now walks restart → wipe.
- M5 multimodal/accessor.py capability_build_failed warning added
  so all four provider accessors log symmetrically (was silent).
- M7 test_knowledge_api parser-absence tests call
  parser_available.cache_clear() around the sys.modules patch so
  the lru_cache doesn't strand a stale True/False.
- M8 cascade_handler_embed_skipped (6 handlers) demoted INFO → DEBUG:
  Tier 1 imports were generating N × 6 handler-info lines per md.
- M10.1 test_drift_scenario_would_raise was a tautology (compared
  two hardcoded string sets, never touched the guard). Now
  monkey-patches BUSINESS_SCHEMAS_WITH_VECTOR to a superset and
  reloads _backfill, proving the import-time RuntimeError fires.
- M11 test_cascade_verbose_position subprocess.run calls gain
  env= — scrubs EVEROS_* from the developer environment so the
  same footgun as round-2 B1 doesn't re-appear inside subprocesses.
- M14 health() -> dict degraded the OpenAPI schema to
  additionalProperties: true. Introduce HealthResponse +
  HealthCapabilities Pydantic models so clients get real field
  shape; docs/openapi.json regenerated.
- M16 routes/knowledge.py:_require_knowledge_capabilities docstring
  claimed cascade.registry.build_handlers still gates
  knowledge_topic/knowledge_document, contradicting
  registry.py:177-194 (gate removed there, moved to HTTP layer).
  Rewritten to describe the current design accurately.

Hermetic env full pytest: 2037 passed / 7 deselected.

Explicitly deferred to followup:
- J1 lazy multimodal client (needs everalgo signature change)
- J2 tier definition (knowledge = Tier 3 whole)
- J5.1/3/4 broader presentation-split refactor
- J6.1/2 backfill dispatch and _backfill_table refactor
- J8 Phase 1 keyset pagination for bulk migration OOM
- M4 flock timeout/waiting log/re-entry (core/persistence refactor)
- M6 UTF-8 codec strategy (BOM/UTF-16/GBK)
- M9 count_by_owner monotonicity (latent, INTERVAL=1 short-circuits)
- M13 --phase all multi-phase combined-outcome test coverage
- M15 PR-marker rationale comments (16 occurrences, all inert)

M12 was refuted (both event names exist).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Kendrick-Song
Kendrick-Song force-pushed the refactor/embed-soft-dependency branch from 67758ec to 20cba06 Compare July 28, 2026 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant