Fix/deep review 2026 07 - #24
Merged
Merged
Conversation
_get_embedding_sync routed every embedding through _fallback_encode whenever an event loop was running - i.e. during every normal orchestration run. Stored vectors were hash-based while memory_reader queries used real model vectors, making HNSW similarity meaningless and silently degrading semantic search to text fallbacks. Prefer embedder.encode_sync (the real model call, which is synchronous under the hood) in async contexts; hash fallback only when no sync encoder exists, now logged at warning level.
Four stacked defects made 'memory_preset:' inert for decay on the default backend (TTLs silently fell back to 2h/168h): - presets nest decay settings under a 'decay' key that _init_decay_config never read -> flatten after preset merge - RedisStackMemoryLogger.memory_decay_config was assigned the raw constructor input, bypassing preset resolution -> use the resolved self.decay_config - the decay mixin read bare long_term_hours/short_term_hours keys that neither presets nor orchestrator config write -> fall back to the default_-prefixed keys (parity with redis_logger) - OrchestratorBase._init_decay_config baked hardcoded defaults that deep-merged over any preset -> only inject defaults when no preset is configured; explicit YAML decay and env vars still override Precedence is now: preset baseline < explicit YAML decay < env vars.
The KNN result score claimed to map distance [0,2] to similarity [1,0] but only clamped the raw distance: a perfect match (distance 0) was reported as similarity 0.0 and readers sorting by similarity_score descending ranked the best matches last. Now similarity = 1 - d/2; missing or NaN scores report 0.0 similarity instead of inheriting a 0.0 'distance' that would read as perfect.
…normalizer Three fallback paths defaulted confidence to the string "0.0" while ResponseNormalizer uses float 0.0, so numeric comparisons in templates and scorers could break for response-only agents. Unified to float. The inline normalizer duplicate in QueueProcessor (only reachable when an engine lacked _response_normalizer, i.e. test doubles) had already drifted from the real one; replaced with lazy attachment of the real ResponseNormalizer and pruned now-unused imports.
_handle_learn built outcome={success: True, quality: <LLM self-reported
confidence>} unconditionally, so the default learn path could never
record a failure - contradicting Brain.learn's record-both-successes-
and-failures design. Workflows can now pass success/quality (e.g. from
an independent scorer agent); defaults unchanged.
Also: consolidate the repeated string-bool coercion into _coerce_bool
(template-rendered "false" was already handled at 4 of 5 sites, now
all 5); fix the feedback() comment that claimed to create a
TRANSFERRED_TO edge (edges connect two skills - feedback only knows the
target context); replace the non-existent brain.execute() in the
package docstring with the real recall->apply-by-LLM->feedback flow.
Note: skill re-embedding at recall is already amortized O(1) by the
AsyncEmbedder LRU cache (encode_sync -> embed -> _cache_get), so no
separate skill-vector cache is needed.
- PAPER_FOLLOWUP_V2: the judge was qwen/qwen3-coder-30b (cross-family), not gpt-oss-20b as stated - verified against all 749 committed judge outputs' _metrics.model and the judge workflow YAMLs. Cite v1's corrected 60.7% pairwise (17/28) instead of the pre-fix 63.3%. Disclose the earlier divergent judge pass retained in results_old/ (+0.06/61.6% vs the reported +0.12/53.8%) as threat 7. - MEMORY_SYSTEM_GUIDE: drop the 1,300 stale lines parked inside an unclosed HTML comment (never rendered; live guide retained). - YAML_CONFIGURATION: remove system_prompt examples - no code reads that field. - AGENT_NODE_TOOL_INDEX: document the registered invariant_validator. - CLAUDE.md: local LLM is direct HTTP (litellm removed); entry-points are declared but never loaded. - server.py docstrings: reflect hardened defaults (local CORS origins, 127.0.0.1 bind, ORKA_API_KEY). - Drop stale litellm filterwarnings; align coverage fail_under 70 -> 80.
- Strip stale '[DEBUG] Bug #N' prefixes from production comments and docstrings (informative content kept). - Remove committed artifacts: .coverage binary, chat.txt (189KB conversation dump), brain_v2_*_results.json at repo root, benchmark_v2_dataset.json.bak; ignore .coverage and *.bak going forward. coverage.xml stays tracked (CI/CodeCov) and is regenerated by the full gated test run.
The committed artifact showed 17.5% - a stale partial run with a refreshed timestamp - while the CI gate requires 80%. Now reflects the actual full-suite result.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @marcosomma, your pull request is larger than the review limit of 150000 diff characters
There was a problem hiding this comment.
Pull request overview
This PR is a deep-review regression hardening pass across OrKa’s orchestration + memory subsystems, aligning scoring semantics (vector distance → similarity), enforcing numeric confidence handling, and making memory preset decay/TTL settings actually take effect end-to-end.
Changes:
- Fix Redis vector search score handling by converting cosine distance to a similarity score in
[0, 1], with targeted regression tests. - Make
memory_presetdecay/TTL settings propagate correctly (flatten preset decay, respect preset vs orchestrator defaults, and readdefault_*_hourskeys consistently). - Remove drift-prone inline response normalization fallback and add regression tests for confidence typing, embedding selection in async contexts, and Brain learn outcome honesty.
Reviewed changes
Copilot reviewed 32 out of 38 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/utils/test_bootstrap_memory_index.py | Updates/extends tests to assert correct distance→similarity conversion and clamping behavior. |
| tests/unit/orchestrator/test_normalizer_orkaresponse.py | Adds regression tests ensuring confidence defaults are numeric and inline normalizer drift doesn’t return. |
| tests/unit/memory/test_memory_preset_decay.py | New regression suite validating preset decay resolution and orchestrator/preset override behavior. |
| tests/unit/memory/redisstack/test_embedding_mixin.py | Adds tests ensuring embeddings in async contexts prefer real model encoding over hash fallback. |
| tests/unit/agents/test_brain_agent.py | Adds tests ensuring Brain “learn” respects success/failure outcomes and boolean coercion works for template strings. |
| pytest.ini | Tightens warning policy by removing a special-case ignore and keeping “never awaited” as errors. |
| pyproject.toml | Removes litellm-specific warning ignore and raises coverage floor to 80%. |
| orka/utils/bootstrap_memory_index.py | Converts Redis vector cosine distance to similarity; handles missing/invalid scores safely. |
| orka/server.py | Updates docs/comments describing safer default CORS behavior and production exposure guidance. |
| orka/orchestrator/metrics.py | Changes confidence default from string to float in previous_outputs builder. |
| orka/orchestrator/execution/response_processor.py | Changes fallback confidence default from string to float in response processing. |
| orka/orchestrator/execution/queue_processor.py | Removes duplicated inline normalization; lazily attaches the real ResponseNormalizer. |
| orka/orchestrator/base.py | Prevents baked TTL defaults from overriding memory_preset during merge. |
| orka/orchestrator/agent_factory.py | Cleans up debug wording around lazy-loading path_executor to avoid circular imports. |
| orka/nodes/path_executor_node.py | Removes debug-tag phrasing in comment without changing behavior. |
| orka/nodes/loop/runner.py | Cleans up debug-tag phrasing in comment about bounding past_loops. |
| orka/nodes/loop/past_loop_builder.py | Clarifies comments about avoiding trace bloat fields. |
| orka/nodes/loop_node.py | Cleans up debug-tag phrasing in comment about parent orchestrator agent extraction. |
| orka/memory/redisstack/embedding_mixin.py | In async contexts, prefers encode_sync (real model path) before hash fallback, with clearer logging. |
| orka/memory/redisstack/decay_mixin.py | Makes decay read default_long_term_hours/default_short_term_hours while preserving agent-level bare overrides. |
| orka/memory/redisstack_logger.py | Ensures memory_decay_config points to the preset-resolved decay_config. |
| orka/memory/base_logger_mixins/config_mixin.py | Flattens preset decay block so preset TTLs are actually visible to decay/metrics readers. |
| orka/brain/brain.py | Clarifies feedback semantics: records transfer outcome on the skill, not via graph edges. |
| orka/brain/init.py | Updates example docstring to show proper feedback loop usage after recall. |
| orka/agents/llm_agents.py | Removes debug-tag phrasing in JSON normalization comments. |
| orka/agents/brain_agent.py | Adds _coerce_bool and uses it to correctly interpret template-rendered success flags; fixes learn quality handling. |
| examples/benchmark_v2/results/PAPER_FOLLOWUP_V2.md | Corrects/clarifies benchmark paper details and threat analysis (numbers, judge model, instability). |
| examples/benchmark_v2/benchmark_v2_dataset.json.bak | Removes large backup dataset file from the repo. |
| docs/YAML_CONFIGURATION.md | Removes system_prompt from YAML examples (documentation alignment). |
| docs/AGENT_NODE_TOOL_INDEX.md | Adds invariant_validator entry to the agent index. |
| CLAUDE.md | Updates architecture notes about LLM endpoints and clarifies entry-point registration isn’t implemented. |
| brain_v2_recipe_results.json | Removes committed results artifact. |
| brain_v2_graphscout_results.json | Removes committed results artifact. |
| .gitignore | Ignores .coverage and *.bak to avoid committing coverage DBs and backup files. |
Comment on lines
341
to
+345
| elif "response" in payload: | ||
| outputs[agent_id] = { | ||
| "response": payload["response"], | ||
| "confidence": payload.get("confidence", "0.0"), | ||
| # float, not "0.0": templates/scorers compare confidence numerically | ||
| "confidence": payload.get("confidence", 0.0), |
Comment on lines
128
to
+132
| if "response" not in payload_out: | ||
| payload_out["response"] = payload_out.get("result") or "" | ||
| if "confidence" not in payload_out: | ||
| payload_out["confidence"] = payload_out.get("confidence", "0.0") | ||
| # float, not "0.0": templates/scorers compare confidence numerically | ||
| payload_out["confidence"] = 0.0 |
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
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.
No description provided.