Repository navigation
feat(chain-learning): prompt-chain learning system with 5 ONEX nodes [OMN-7032] - #1101
Merged
Merged
Conversation
…des and tests Implements a chain-learning cache layer: verified workflow trajectories are stored in Qdrant and replayed on similar future prompts, shortcutting full re-exploration. Nodes: - node_chain_retrieval_effect (EFFECT): embed prompt, query Qdrant - node_chain_replay_compute (COMPUTE): adapt cached chain to new context - node_chain_verify_reducer (REDUCER): FSM tracking verification lifecycle - node_chain_store_effect (EFFECT): persist verified chain to Qdrant - node_chain_orchestrator (ORCHESTRATOR): coordinate the full workflow Tests: 87 unit tests covering models, all handlers (orchestrator + effect + compute + reducer), contract YAML validation, FSM transitions, and error paths.
… mypy strict Qdrant ScoredPoint objects are returned as `object` from the protocol. Use getattr instead of direct attribute access to avoid mypy attr-defined errors.
📝 WalkthroughWalkthroughAdds a chain learning workflow: an orchestrator, a replay compute node, retrieval and store effect nodes, a verify reducer, associated Pydantic models/protocols, handlers, YAML contracts, and unit tests validating contracts, handlers, models, and validator thresholds. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Orchestrator as Chain Orchestrator<br/>(NodeChainOrchestrator)
participant Retrieval as Retrieval<br/>(NodeChainRetrievalEffect)
participant Replay as Replay<br/>(NodeChainReplayCompute)
participant Verify as Verify<br/>(NodeChainVerifyReducer)
participant Store as Store<br/>(NodeChainStoreEffect)
Client->>Orchestrator: Learn Command (prompt_text, correlation_id)
rect rgba(100, 150, 255, 0.5)
Note over Orchestrator,Retrieval: INITIATE & RETRIEVE
Orchestrator->>Retrieval: Embed prompt, search Qdrant
Retrieval-->>Orchestrator: RetrievalResult (matches or empty)
end
alt Hit Found (similarity ≥ threshold)
rect rgba(100, 200, 100, 0.5)
Note over Orchestrator,Replay: REPLAY PATH
Orchestrator->>Replay: Adapt cached chain to new context
Replay-->>Orchestrator: ReplayResult (adapted_steps, confidence)
Orchestrator->>Verify: Verify adapted result
Verify-->>Orchestrator: Verified state/result
end
else Miss or Low Confidence
rect rgba(200, 150, 100, 0.5)
Note over Orchestrator: EXPLORE / FALLBACK
Orchestrator->>Verify: Mark for explore or fallback
Verify-->>Orchestrator: Updated state
end
end
rect rgba(150, 100, 200, 0.5)
Note over Orchestrator,Store: STORE VERIFIED CHAIN
Orchestrator->>Store: Persist chain & embedding to Qdrant
Store-->>Orchestrator: StoreResult (success/failure)
end
Orchestrator-->>Client: LearnResult (path_taken, chain_ref, success, latency)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (12)
src/omnibase_infra/nodes/node_chain_verify_reducer/models/__init__.py (1)
3-3: Docstring is slightly misleading about re-exports.Line 3 says this module re-exports orchestrator models, but no symbols are actually re-exported. Consider either adding explicit exports or adjusting the docstring.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnibase_infra/nodes/node_chain_verify_reducer/models/__init__.py` at line 3, The module docstring claims to re-export orchestrator models but currently exports nothing; update the module to either import and re-export the intended symbols (e.g., import specific model names from the orchestrator module and add them to __all__ or assign them in this package namespace) or change the docstring to accurately reflect there are no re-exports; target the __init__.py in the node_chain_verify_reducer.models package and adjust by adding explicit imports of the orchestrator model classes/functions and an __all__ listing, or by editing the docstring text to remove the "re-exports" claim.src/omnibase_infra/nodes/node_chain_store_effect/models/__init__.py (1)
3-3: Docstring overstates behavior (re-exports) without actual exports.Consider either adding explicit re-exports or simplifying the module docstring to avoid drift.
Proposed docstring-only fix
-"""Chain store effect models (re-exports from orchestrator models).""" +"""Chain store effect models package."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnibase_infra/nodes/node_chain_store_effect/models/__init__.py` at line 3, The module docstring claims "re-exports from orchestrator models" but the package does not actually re-export anything; fix by either (a) adding explicit re-exports: import the desired symbols from the orchestrator models module and expose them via an __all__ tuple in this module (e.g., import X, Y, Z from orchestrator.models and set __all__ = ["X","Y","Z"]) so callers can import from node_chain_store_effect.models, or (b) simplify the module docstring to remove the "re-exports" claim and accurately describe the module's current purpose; update the module-level docstring in __init__.py accordingly.src/omnibase_infra/nodes/node_chain_retrieval_effect/models/__init__.py (1)
3-3: Docstring says “re-exports” but no re-export surface is defined.Tighten the wording (or add explicit exports) so this module remains self-describing.
Proposed docstring-only fix
-"""Chain retrieval effect models (re-exports from orchestrator models).""" +"""Chain retrieval effect models package."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnibase_infra/nodes/node_chain_retrieval_effect/models/__init__.py` at line 3, The module docstring in node_chain_retrieval_effect.models claims "re-exports from orchestrator models" but no re-exports are provided; either update the docstring to accurately describe the module (e.g., remove "re-exports" wording) or add explicit re-exports by importing the intended symbols from the orchestrator models and exposing them via __all__ (for example import the specific classes/functions from the orchestrator models and list them in __all__). Locate the package initializer (__init__.py) for node_chain_retrieval_effect.models, decide which orchestrator model symbols should be forwarded, add the import statements and an __all__ tuple, or change the docstring to a concise accurate description of the module contents.src/omnibase_infra/nodes/node_chain_orchestrator/handlers/handler_chain_learn_initiate.py (1)
54-59: Keepsimilarity_thresholdasfloatin the internal dispatch payload.Converting to
strhere introduces unnecessary type drift for downstream handlers.Suggested fix
return { "prompt_text": prompt_text, "workflow_ref": workflow_ref, "correlation_id": str(correlation_id), - "similarity_threshold": str(similarity_threshold), + "similarity_threshold": similarity_threshold, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnibase_infra/nodes/node_chain_orchestrator/handlers/handler_chain_learn_initiate.py` around lines 54 - 59, The returned dispatch payload in handler_chain_learn_initiate.py is converting similarity_threshold to a string which causes type drift for downstream handlers; change the return so "similarity_threshold" remains a float (remove the str(...) around similarity_threshold) while keeping "correlation_id" as str, i.e., return the dict with keys "prompt_text", "workflow_ref", "correlation_id": str(correlation_id), and "similarity_threshold": similarity_threshold so downstream functions that expect a float (e.g., any consumers of this dispatch payload) receive the correct type.src/omnibase_infra/nodes/node_chain_store_effect/handlers/handler_chain_store.py (1)
68-77: Dynamic import placement could be moved to module level.The
qdrant_client.models.PointStructimport insidehandle()adds overhead on every call. Consider moving it to module level with a try/except fallback or lazy initialization, since this handler is specifically for Qdrant operations.📝 Suggested refactor
+try: + from qdrant_client.models import PointStruct +except ImportError: + PointStruct = None # type: ignore[misc, assignment] + COLLECTION_NAME = "onex_chain_trajectories" class HandlerChainStore: ... async def handle( ... ) -> ModelChainStoreResult: ... - try: - from qdrant_client.models import PointStruct - except ImportError: - logger.exception("qdrant-client not installed") - return ModelChainStoreResult(...) + if PointStruct is None: + logger.error("qdrant-client not installed") + return ModelChainStoreResult( + correlation_id=correlation_id, + chain_id=chain_entry.chain_id, + success=False, + error_message="qdrant-client is required for chain storage", + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnibase_infra/nodes/node_chain_store_effect/handlers/handler_chain_store.py` around lines 68 - 77, Move the qdrant PointStruct import out of handle() and into the module top: add a module-level try/except that attempts "from qdrant_client.models import PointStruct" and on ImportError sets PointStruct = None (and logs once). In the handle() function, replace the inline dynamic import with a check that PointStruct is not None; if it is None return the same ModelChainStoreResult error (using correlation_id and chain_entry.chain_id) so behavior is preserved. This avoids per-call import overhead while keeping the same error path when qdrant-client is not installed.src/omnibase_infra/nodes/node_chain_replay_compute/handlers/handler_chain_replay.py (1)
74-79: Substring replacement could cause unintended substitutions.The simple
str.replace()approach may cause unintended matches if context keys are substrings of other content in the operation field. For example, ifkey="id"andoperation="validate_id", the replacement would producevalidate_<value>.Consider using word boundary matching or a placeholder syntax (e.g.,
{{key}}) for safer substitution.📝 Safer substitution approach
for key, value in context.items(): - if key in new_operation: - new_operation = new_operation.replace(key, value) + placeholder = f"{{{{{key}}}}}" # e.g., {{key}} + if placeholder in new_operation: + new_operation = new_operation.replace(placeholder, value) adaptations_made.append( f"step[{step.step_index}]: substituted {key}" )This requires the cached operations to use
{{key}}placeholders, making substitution explicit and safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnibase_infra/nodes/node_chain_replay_compute/handlers/handler_chain_replay.py` around lines 74 - 79, The current loop in handler_chain_replay.py uses naive substring replacement on new_operation which can accidentally replace parts of other identifiers (e.g., key "id" replacing inside "validate_id"); change the substitution to a safer placeholder-based or word-boundary approach: treat context keys as explicit placeholders like {{key}} (or use regex with \b boundaries) and only replace those exact tokens when iterating over context, updating the adaptations_made message referencing step.step_index and new_operation accordingly so unintended substrings are not replaced.src/omnibase_infra/nodes/node_chain_orchestrator/contract.yaml (1)
93-95: Consider addingdepends_onfor the verify node.The
verifynode doesn't declare any dependencies, but logically it should depend on the outcome of eitherreplayorretrievebased on the hit/miss path. Whilesequentialexecution mode may handle ordering implicitly, explicit dependencies would better document the workflow structure.📝 Suggested improvement
- node_id: "verify" node_type: REDUCER_GENERIC description: "FSM tracking verification state" + depends_on: ["replay"]Or if it should support both paths:
depends_on: ["replay", "retrieve"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnibase_infra/nodes/node_chain_orchestrator/contract.yaml` around lines 93 - 95, The verify node (node_id: "verify", node_type: REDUCER_GENERIC) lacks explicit dependencies which obscures its relationship to the replay/retrieve outcomes; add a depends_on entry referencing the upstream nodes (e.g., depends_on: ["replay","retrieve"] or the single correct upstream node) so the workflow explicitly documents that verify runs after those nodes and clarifies the hit/miss paths.src/omnibase_infra/nodes/node_chain_retrieval_effect/handlers/handler_chain_retrieval.py (2)
177-199: Collection auto-creation is best-effort with broad exception handling.The
_ensure_collectionmethod catchesImportError,OSError, andRuntimeErrorbut logs only a warning. This is appropriate for a best-effort approach, but consider whethercreate_collectionfailures should propagate in production scenarios where the collection is required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnibase_infra/nodes/node_chain_retrieval_effect/handlers/handler_chain_retrieval.py` around lines 177 - 199, The try/except in _ensure_collection currently swallows ImportError, OSError, and RuntimeError and only warns, which can hide critical failures creating COLLECTION_NAME; update _ensure_collection to (1) catch and log the full exception details (include exception message/stack) when collection creation fails (referencing self._qdrant_client.create_collection and collection_exists), and (2) make failure behavior configurable or stricter in production by re-raising the exception (or raising a new RuntimeError) when a flag like self.fail_on_collection_error (or an injected config) is true so callers can decide to abort instead of continuing silently.
138-144: Consider adding a clarifying comment about the distance calculation.The calculation
distance=1.0 - scoreis mathematically correct: Qdrant'sDistance.COSINEmetric returns cosine similarity (range 0-1, where 1 is identical), so converting it to distance by subtracting from 1 is the appropriate inverse relationship. A comment explaining this conversion would improve code clarity for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnibase_infra/nodes/node_chain_retrieval_effect/handlers/handler_chain_retrieval.py` around lines 138 - 144, Add a brief clarifying comment above the ModelChainMatch construction explaining that Qdrant returns cosine similarity (score in 0..1 where 1 means identical) and we convert it to a distance by using distance = 1.0 - score; reference the variables used here (score, similarity_score, distance, and the ModelChainMatch instantiation) so future maintainers understand the inverse relationship.tests/unit/nodes/node_chain_learning/test_handlers.py (1)
7-7: Unused import:timezoneThe
timezoneimport is unused; onlyUTCis used on lines 61 and 204.🧹 Proposed fix
-from datetime import UTC, datetime, timezone +from datetime import UTC, datetime🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/nodes/node_chain_learning/test_handlers.py` at line 7, The import statement "from datetime import UTC, datetime, timezone" includes an unused symbol `timezone`; remove `timezone` from that import so it reads only the used symbols (UTC and datetime) to eliminate the unused-import warning in the test module (look for the import line and references to UTC on lines around 61 and 204).tests/unit/nodes/node_chain_learning/test_contracts.py (2)
17-17: Relative path traversal may be fragile.The path
Path(__file__).resolve().parents[4]assumes a fixed directory depth. If the test file is moved, this will break silently. Consider using a more robust approach.🧹 Alternative approach using project root marker
def _find_project_root() -> Path: """Find project root by looking for pyproject.toml or .git.""" current = Path(__file__).resolve().parent for parent in [current, *current.parents]: if (parent / "pyproject.toml").exists() or (parent / ".git").exists(): return parent raise RuntimeError("Could not find project root") _NODES_DIR = _find_project_root() / "src" / "omnibase_infra" / "nodes"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/nodes/node_chain_learning/test_contracts.py` at line 17, The test uses a fragile fixed-depth path via _NODES_DIR = Path(__file__).resolve().parents[4]..., which will break if files move; replace this with a robust project-root discovery function (e.g., _find_project_root) that walks parents from Path(__file__).resolve().parent looking for a project marker like pyproject.toml or .git and then set _NODES_DIR = _find_project_root() / "src" / "omnibase_infra" / "nodes"; update any imports or references that use _NODES_DIR accordingly.
39-40: Specify explicit encoding for file operations.The
open()call lacks an explicit encoding parameter. On some systems, the default locale encoding may not be UTF-8.🧹 Proposed fix
- with open(contract_path) as f: + with open(contract_path, encoding="utf-8") as f: return yaml.safe_load(f)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/nodes/node_chain_learning/test_contracts.py` around lines 39 - 40, The open() call that reads contract_path should explicitly set the file encoding to UTF-8 to avoid locale-dependent behavior; update the code that does "with open(contract_path) as f:" to include encoding="utf-8" when opening the file so yaml.safe_load(f) always reads a UTF-8 encoded file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/omnibase_infra/nodes/node_chain_orchestrator/handlers/handler_chain_replay_complete.py`:
- Around line 53-57: The replay success branch in
handler_chain_replay_complete.py currently returns an action "store" and thus
bypasses the verification phase; change the returned action to trigger
verification first (e.g., "verify") and include the same metadata (use
replay_result.confidence and replay_result.adaptation_summary) so the
verification handler can run and later route to store on success; update any
callers or downstream routing that expect an "action" value if needed to accept
"verify" before "store".
In
`@src/omnibase_infra/nodes/node_chain_orchestrator/handlers/handler_chain_retrieval_complete.py`:
- Around line 51-65: The returned payloads in handler_chain_retrieval_complete
currently set "is_hit" to the string literals "true"/"false" (see the two return
dicts in the shown block); change those to real booleans True and False
respectively so downstream consumers receive proper booleans (leave other fields
like "path" and "chain_id" as-is); update both the hit case (where you return
{"path":"replay", "chain_id": str(best_match.chain_entry.chain_id), "is_hit":
True}) and the miss case ({"path":"explore", "is_hit": False}) in
handler_chain_retrieval_complete.
In
`@src/omnibase_infra/nodes/node_chain_orchestrator/handlers/handler_chain_store_complete.py`:
- Around line 49-64: The handler is returning a dict with wrong keys/types;
update the return shaped to match the ModelChainLearnResult contract in
handler_chain_store_complete (use ModelChainLearnResult or a dict with exact
fields). Map store_result.chain_id -> chain_ref (as string) not chain_id, rename
error -> error_message, and set success to a boolean (True/False) instead of
string; also populate the required fields correlation_id, path_taken,
retrieval_latency_ms, and total_latency_ms (use values from correlation_id
variable, store_result.path or []/[], latency values from store_result or
compute defaults if missing). Prefer constructing and returning a
ModelChainLearnResult instance (or a dict matching its schema) so types and
field names align with the contract.
In
`@src/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_entry.py`:
- Line 18: The model_config currently sets ConfigDict(frozen=True,
extra="forbid") but is missing from_attributes=True; update the ConfigDict for
the Pydantic model (the model_config variable in model_chain_entry.py) to
include from_attributes=True so it becomes ConfigDict(frozen=True,
extra="forbid", from_attributes=True") to satisfy repository-wide Pydantic
requirements.
In
`@src/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_learn_command.py`:
- Line 15: The Pydantic model config for model_chain_learn_command.py is missing
from_attributes=True; update the model_config assignment (the ConfigDict
instance named model_config) to include from_attributes=True alongside
frozen=True and extra="forbid" so the model follows the repository contract for
Pydantic models (thread safety and strict validation).
In
`@src/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_learn_result.py`:
- Line 16: The ConfigDict for this Pydantic model is missing the required
from_attributes flag; update the model_config assignment (the ConfigDict in
model_chain_learn_result.py named model_config) to include from_attributes=True
(i.e., set ConfigDict(frozen=True, extra="forbid", from_attributes=True)) so the
model complies with the Pydantic model_* file guidelines.
In
`@src/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_match.py`:
- Line 15: The ConfigDict for the Pydantic model (the variable model_config
using ConfigDict) is missing from_attributes=True; update the ConfigDict call in
model_chain_match.py so that model_config = ConfigDict(frozen=True,
extra="forbid", from_attributes=True) to comply with the guideline; ensure you
only modify the ConfigDict arguments for the existing model_config symbol.
In
`@src/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_replay_input.py`:
- Line 17: The ConfigDict for the Pydantic model is missing
from_attributes=True; update the ConfigDict assigned to model_config in
model_chain_replay_input (i.e., replace or modify the current model_config =
ConfigDict(frozen=True, extra="forbid") to include from_attributes=True) so the
model follows the required pattern (frozen=True, extra="forbid",
from_attributes=True).
In
`@src/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_replay_result.py`:
- Line 17: The model ConfigDict for this Pydantic model currently sets
frozen=True and extra="forbid" but is missing from_attributes=True; update the
model_config declaration (the ConfigDict assigned to model_config in this
module) to include from_attributes=True so it reads ConfigDict(frozen=True,
extra="forbid", from_attributes=True) to comply with the model_* guidelines.
In
`@src/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_retrieval_result.py`:
- Line 17: The ConfigDict for the Pydantic model in this module is missing
from_attributes=True; update the module's model_config (the ConfigDict
instantiation named model_config) to include from_attributes=True so it reads
ConfigDict(frozen=True, extra="forbid", from_attributes=True) to comply with
repository Pydantic model requirements.
In `@src/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_step.py`:
- Line 13: The ConfigDict for this Pydantic model (model_config) is missing
from_attributes=True; update the model_config definition in model_chain_step.py
to include from_attributes=True alongside frozen=True and extra="forbid" so it
matches other model_*.py files and the repo coding guidelines.
In
`@src/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_store_request.py`:
- Line 17: The model's ConfigDict is missing from_attributes=True; update the
ConfigDict instantiation for the model_config symbol in
model_chain_store_request (where model_config = ConfigDict(...)) to include
from_attributes=True so it reads ConfigDict(frozen=True, extra="forbid",
from_attributes=True), ensuring the Pydantic model follows the required
guidelines.
In
`@src/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_store_result.py`:
- Line 15: The model's Pydantic configuration (the variable model_config) is
missing from_attributes=True; update the ConfigDict assignment for model_config
(used by the model class in model_chain_store_result) to include
from_attributes=True alongside frozen=True and extra="forbid" so the model
complies with the repo's Pydantic contract.
In
`@src/omnibase_infra/nodes/node_chain_verify_reducer/handlers/handler_chain_verify.py`:
- Around line 41-50: The handler's declared classification (HandlerChainVerify
via its handler_type/handler_category properties) doesn't match the entry in
contract.handler_routing (pipeline expects this handler to be registered as
"hybrid"), so update either the contract routing or the handler classification
so they align: locate the HandlerChainVerify class and the Enum values it
returns (handler_type, handler_category), then change the return value(s) to the
Enum variant that matches the routing entry used in contract.handler_routing (or
update the contract.handler_routing key/value to the Enum currently returned).
Ensure the symbol names involved are HandlerChainVerify, handler_type,
handler_category, EnumHandlerType, EnumHandlerTypeCategory and the contract
mapping contract.handler_routing so the handler verdict and routing registration
are identical.
In `@tests/unit/nodes/node_chain_learning/test_models.py`:
- Around line 58-81: The tests test_extra_forbid and
test_negative_step_index_rejected are instantiating ModelChainStep with
node_name instead of the model's required field node_ref, which lets failures be
caused by missing required fields; update both test instantiations to pass
node_ref (and keep all other required args: step_index, operation, input_hash,
output_hash, duration_ms, event_topic) so the tests exercise the intended
constraints (forbidden extra field and negative step_index) against
ModelChainStep rather than failing early due to a missing node_ref.
---
Nitpick comments:
In `@src/omnibase_infra/nodes/node_chain_orchestrator/contract.yaml`:
- Around line 93-95: The verify node (node_id: "verify", node_type:
REDUCER_GENERIC) lacks explicit dependencies which obscures its relationship to
the replay/retrieve outcomes; add a depends_on entry referencing the upstream
nodes (e.g., depends_on: ["replay","retrieve"] or the single correct upstream
node) so the workflow explicitly documents that verify runs after those nodes
and clarifies the hit/miss paths.
In
`@src/omnibase_infra/nodes/node_chain_orchestrator/handlers/handler_chain_learn_initiate.py`:
- Around line 54-59: The returned dispatch payload in
handler_chain_learn_initiate.py is converting similarity_threshold to a string
which causes type drift for downstream handlers; change the return so
"similarity_threshold" remains a float (remove the str(...) around
similarity_threshold) while keeping "correlation_id" as str, i.e., return the
dict with keys "prompt_text", "workflow_ref", "correlation_id":
str(correlation_id), and "similarity_threshold": similarity_threshold so
downstream functions that expect a float (e.g., any consumers of this dispatch
payload) receive the correct type.
In
`@src/omnibase_infra/nodes/node_chain_replay_compute/handlers/handler_chain_replay.py`:
- Around line 74-79: The current loop in handler_chain_replay.py uses naive
substring replacement on new_operation which can accidentally replace parts of
other identifiers (e.g., key "id" replacing inside "validate_id"); change the
substitution to a safer placeholder-based or word-boundary approach: treat
context keys as explicit placeholders like {{key}} (or use regex with \b
boundaries) and only replace those exact tokens when iterating over context,
updating the adaptations_made message referencing step.step_index and
new_operation accordingly so unintended substrings are not replaced.
In
`@src/omnibase_infra/nodes/node_chain_retrieval_effect/handlers/handler_chain_retrieval.py`:
- Around line 177-199: The try/except in _ensure_collection currently swallows
ImportError, OSError, and RuntimeError and only warns, which can hide critical
failures creating COLLECTION_NAME; update _ensure_collection to (1) catch and
log the full exception details (include exception message/stack) when collection
creation fails (referencing self._qdrant_client.create_collection and
collection_exists), and (2) make failure behavior configurable or stricter in
production by re-raising the exception (or raising a new RuntimeError) when a
flag like self.fail_on_collection_error (or an injected config) is true so
callers can decide to abort instead of continuing silently.
- Around line 138-144: Add a brief clarifying comment above the ModelChainMatch
construction explaining that Qdrant returns cosine similarity (score in 0..1
where 1 means identical) and we convert it to a distance by using distance = 1.0
- score; reference the variables used here (score, similarity_score, distance,
and the ModelChainMatch instantiation) so future maintainers understand the
inverse relationship.
In `@src/omnibase_infra/nodes/node_chain_retrieval_effect/models/__init__.py`:
- Line 3: The module docstring in node_chain_retrieval_effect.models claims
"re-exports from orchestrator models" but no re-exports are provided; either
update the docstring to accurately describe the module (e.g., remove
"re-exports" wording) or add explicit re-exports by importing the intended
symbols from the orchestrator models and exposing them via __all__ (for example
import the specific classes/functions from the orchestrator models and list them
in __all__). Locate the package initializer (__init__.py) for
node_chain_retrieval_effect.models, decide which orchestrator model symbols
should be forwarded, add the import statements and an __all__ tuple, or change
the docstring to a concise accurate description of the module contents.
In
`@src/omnibase_infra/nodes/node_chain_store_effect/handlers/handler_chain_store.py`:
- Around line 68-77: Move the qdrant PointStruct import out of handle() and into
the module top: add a module-level try/except that attempts "from
qdrant_client.models import PointStruct" and on ImportError sets PointStruct =
None (and logs once). In the handle() function, replace the inline dynamic
import with a check that PointStruct is not None; if it is None return the same
ModelChainStoreResult error (using correlation_id and chain_entry.chain_id) so
behavior is preserved. This avoids per-call import overhead while keeping the
same error path when qdrant-client is not installed.
In `@src/omnibase_infra/nodes/node_chain_store_effect/models/__init__.py`:
- Line 3: The module docstring claims "re-exports from orchestrator models" but
the package does not actually re-export anything; fix by either (a) adding
explicit re-exports: import the desired symbols from the orchestrator models
module and expose them via an __all__ tuple in this module (e.g., import X, Y, Z
from orchestrator.models and set __all__ = ["X","Y","Z"]) so callers can import
from node_chain_store_effect.models, or (b) simplify the module docstring to
remove the "re-exports" claim and accurately describe the module's current
purpose; update the module-level docstring in __init__.py accordingly.
In `@src/omnibase_infra/nodes/node_chain_verify_reducer/models/__init__.py`:
- Line 3: The module docstring claims to re-export orchestrator models but
currently exports nothing; update the module to either import and re-export the
intended symbols (e.g., import specific model names from the orchestrator module
and add them to __all__ or assign them in this package namespace) or change the
docstring to accurately reflect there are no re-exports; target the __init__.py
in the node_chain_verify_reducer.models package and adjust by adding explicit
imports of the orchestrator model classes/functions and an __all__ listing, or
by editing the docstring text to remove the "re-exports" claim.
In `@tests/unit/nodes/node_chain_learning/test_contracts.py`:
- Line 17: The test uses a fragile fixed-depth path via _NODES_DIR =
Path(__file__).resolve().parents[4]..., which will break if files move; replace
this with a robust project-root discovery function (e.g., _find_project_root)
that walks parents from Path(__file__).resolve().parent looking for a project
marker like pyproject.toml or .git and then set _NODES_DIR =
_find_project_root() / "src" / "omnibase_infra" / "nodes"; update any imports or
references that use _NODES_DIR accordingly.
- Around line 39-40: The open() call that reads contract_path should explicitly
set the file encoding to UTF-8 to avoid locale-dependent behavior; update the
code that does "with open(contract_path) as f:" to include encoding="utf-8" when
opening the file so yaml.safe_load(f) always reads a UTF-8 encoded file.
In `@tests/unit/nodes/node_chain_learning/test_handlers.py`:
- Line 7: The import statement "from datetime import UTC, datetime, timezone"
includes an unused symbol `timezone`; remove `timezone` from that import so it
reads only the used symbols (UTC and datetime) to eliminate the unused-import
warning in the test module (look for the import line and references to UTC on
lines around 61 and 204).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33a6189b-fe78-4c52-b6b5-b22f26484aea
⛔ Files ignored due to path filters (1)
src/omnibase_infra/enums/generated/enum_omnibase_infra_topic.pyis excluded by!**/generated/**
📒 Files selected for processing (53)
src/omnibase_infra/nodes/node_chain_orchestrator/__init__.pysrc/omnibase_infra/nodes/node_chain_orchestrator/contract.yamlsrc/omnibase_infra/nodes/node_chain_orchestrator/handlers/__init__.pysrc/omnibase_infra/nodes/node_chain_orchestrator/handlers/handler_chain_learn_initiate.pysrc/omnibase_infra/nodes/node_chain_orchestrator/handlers/handler_chain_replay_complete.pysrc/omnibase_infra/nodes/node_chain_orchestrator/handlers/handler_chain_retrieval_complete.pysrc/omnibase_infra/nodes/node_chain_orchestrator/handlers/handler_chain_store_complete.pysrc/omnibase_infra/nodes/node_chain_orchestrator/models/__init__.pysrc/omnibase_infra/nodes/node_chain_orchestrator/models/enum_chain_verify_state.pysrc/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_entry.pysrc/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_learn_command.pysrc/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_learn_result.pysrc/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_match.pysrc/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_replay_input.pysrc/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_replay_result.pysrc/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_retrieval_result.pysrc/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_step.pysrc/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_store_request.pysrc/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_store_result.pysrc/omnibase_infra/nodes/node_chain_orchestrator/models/protocol_chain_embedding_client.pysrc/omnibase_infra/nodes/node_chain_orchestrator/models/protocol_chain_vector_client.pysrc/omnibase_infra/nodes/node_chain_orchestrator/node.pysrc/omnibase_infra/nodes/node_chain_replay_compute/__init__.pysrc/omnibase_infra/nodes/node_chain_replay_compute/contract.yamlsrc/omnibase_infra/nodes/node_chain_replay_compute/handlers/__init__.pysrc/omnibase_infra/nodes/node_chain_replay_compute/handlers/handler_chain_replay.pysrc/omnibase_infra/nodes/node_chain_replay_compute/models/__init__.pysrc/omnibase_infra/nodes/node_chain_replay_compute/node.pysrc/omnibase_infra/nodes/node_chain_retrieval_effect/__init__.pysrc/omnibase_infra/nodes/node_chain_retrieval_effect/contract.yamlsrc/omnibase_infra/nodes/node_chain_retrieval_effect/handlers/__init__.pysrc/omnibase_infra/nodes/node_chain_retrieval_effect/handlers/handler_chain_retrieval.pysrc/omnibase_infra/nodes/node_chain_retrieval_effect/models/__init__.pysrc/omnibase_infra/nodes/node_chain_retrieval_effect/node.pysrc/omnibase_infra/nodes/node_chain_store_effect/__init__.pysrc/omnibase_infra/nodes/node_chain_store_effect/contract.yamlsrc/omnibase_infra/nodes/node_chain_store_effect/handlers/__init__.pysrc/omnibase_infra/nodes/node_chain_store_effect/handlers/handler_chain_store.pysrc/omnibase_infra/nodes/node_chain_store_effect/models/__init__.pysrc/omnibase_infra/nodes/node_chain_store_effect/node.pysrc/omnibase_infra/nodes/node_chain_verify_reducer/__init__.pysrc/omnibase_infra/nodes/node_chain_verify_reducer/contract.yamlsrc/omnibase_infra/nodes/node_chain_verify_reducer/handlers/__init__.pysrc/omnibase_infra/nodes/node_chain_verify_reducer/handlers/handler_chain_verify.pysrc/omnibase_infra/nodes/node_chain_verify_reducer/models/__init__.pysrc/omnibase_infra/nodes/node_chain_verify_reducer/node.pysrc/omnibase_infra/validation/infra_validators.pytests/unit/nodes/node_chain_learning/__init__.pytests/unit/nodes/node_chain_learning/test_contracts.pytests/unit/nodes/node_chain_learning/test_effect_handlers.pytests/unit/nodes/node_chain_learning/test_handlers.pytests/unit/nodes/node_chain_learning/test_models.pytests/unit/validation/test_validator_defaults.py
jonahgabriel
enabled auto-merge
April 2, 2026 03:03
jonahgabriel
changed the title
…[OMN-7032] - Add handler_routing to verify reducer contract (fixes Handler Contract Compliance) - Add from_attributes=True to all 10 chain model ConfigDicts - Add ProtocolChainEmbeddingClient and ProtocolChainVectorClient to protocol allowlist (fixes protocol ownership tests) - Fix string booleans to real booleans in retrieval/store handlers - Route replay success to verify step instead of directly to store - Fix node_name -> node_ref in test_models.py assertions - Remove unused imports (timezone, AsyncMock, MagicMock) from test files
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/unit/nodes/node_chain_learning/test_handlers.py (2)
117-127: Rename test to match behavior (verify, notstore).The test name says “stores” but validates
action == "verify". Renaming avoids misleading intent.Suggested rename
- async def test_high_confidence_stores(self) -> None: + async def test_high_confidence_verifies(self) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/nodes/node_chain_learning/test_handlers.py` around lines 117 - 127, Rename the test function test_high_confidence_stores to reflect its asserted behavior (e.g., test_high_confidence_verifies) so the name matches the expectation that HandlerChainReplayComplete returns action == "verify"; update the test function name where ModelChainReplayResult is used and where result["action"] == "verify" is asserted to keep intent clear.
116-139: Add a threshold-boundary test at exactly0.6.Current tests cover high/low confidence, but not the inclusive boundary (
>= MIN_REPLAY_CONFIDENCE). A dedicated boundary test will lock expected behavior.Suggested additional test
class TestHandlerChainReplayComplete: @@ async def test_low_confidence_fallback(self) -> None: handler = HandlerChainReplayComplete() @@ result = await handler.handle(replay, uuid4()) assert result["action"] == "fallback" + + `@pytest.mark.asyncio` + async def test_threshold_confidence_verifies(self) -> None: + handler = HandlerChainReplayComplete() + replay = ModelChainReplayResult( + correlation_id=uuid4(), + adapted_steps=(_make_step(),), + adaptation_summary="Boundary case", + confidence=0.6, + ) + result = await handler.handle(replay, uuid4()) + assert result["action"] == "verify"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/nodes/node_chain_learning/test_handlers.py` around lines 116 - 139, Add a boundary unit test that verifies HandlerChainReplayComplete treats confidence exactly at the threshold as "verify": create a new async test similar to test_high_confidence_stores but set the ModelChainReplayResult confidence to MIN_REPLAY_CONFIDENCE (or 0.6 if the constant isn't imported) and assert handler.handle(...) returns result["action"] == "verify"; reference HandlerChainReplayComplete and ModelChainReplayResult so the test mirrors the existing tests (use uuid4() and _make_step() as in the other cases).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/omnibase_infra/nodes/node_chain_orchestrator/handlers/handler_chain_replay_complete.py`:
- Around line 53-67: The handler currently returns plain dicts from handle() but
the contract requires returning a ModelChainStoreRequest instance; change
handle() to construct and return a ModelChainStoreRequest object (import it at
top) with the same fields you were placing in the dict (map confidence,
adaptation_summary -> summary, action -> action, and include correlation_id if
the model has it) for both the "verify" and "fallback" branches instead of
returning dicts; also update the function/class docstrings that currently
mention "store or fallback logic" to explicitly reference the "verify" and
"fallback" actions to match the payloads and contract-driven routing.
In `@src/omnibase_infra/nodes/node_chain_verify_reducer/contract.yaml`:
- Around line 121-140: The contract is missing event_bus topic/schema
declarations required by the node-contract rule; update the YAML to add an
event_bus section that declares publish_topics and/or subscribe_topics for this
reducer (referencing the chain.verify operation/HandlerChainVerify flow and the
ModelReducerInput payload), include topic names and schema/module references for
the event messages used by the handler (e.g., the verification-result or
reducer-event schemas) so Kafka topics and event schemas are defined here rather
than in code; ensure the new event_bus keys follow the existing contract shape
and reference the correct schema modules for the messages consumed/produced by
HandlerChainVerify.
---
Nitpick comments:
In `@tests/unit/nodes/node_chain_learning/test_handlers.py`:
- Around line 117-127: Rename the test function test_high_confidence_stores to
reflect its asserted behavior (e.g., test_high_confidence_verifies) so the name
matches the expectation that HandlerChainReplayComplete returns action ==
"verify"; update the test function name where ModelChainReplayResult is used and
where result["action"] == "verify" is asserted to keep intent clear.
- Around line 116-139: Add a boundary unit test that verifies
HandlerChainReplayComplete treats confidence exactly at the threshold as
"verify": create a new async test similar to test_high_confidence_stores but set
the ModelChainReplayResult confidence to MIN_REPLAY_CONFIDENCE (or 0.6 if the
constant isn't imported) and assert handler.handle(...) returns result["action"]
== "verify"; reference HandlerChainReplayComplete and ModelChainReplayResult so
the test mirrors the existing tests (use uuid4() and _make_step() as in the
other cases).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c69daf3a-5360-472b-8550-a7d717a4835e
📒 Files selected for processing (17)
src/omnibase_infra/nodes/node_chain_orchestrator/handlers/handler_chain_replay_complete.pysrc/omnibase_infra/nodes/node_chain_orchestrator/handlers/handler_chain_retrieval_complete.pysrc/omnibase_infra/nodes/node_chain_orchestrator/handlers/handler_chain_store_complete.pysrc/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_entry.pysrc/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_learn_command.pysrc/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_learn_result.pysrc/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_match.pysrc/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_replay_input.pysrc/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_replay_result.pysrc/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_retrieval_result.pysrc/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_step.pysrc/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_store_request.pysrc/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_store_result.pysrc/omnibase_infra/nodes/node_chain_verify_reducer/contract.yamltests/unit/contracts/test_protocol_ownership.pytests/unit/nodes/node_chain_learning/test_handlers.pytests/unit/nodes/node_chain_learning/test_models.py
✅ Files skipped from review due to trivial changes (11)
- src/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_store_result.py
- src/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_step.py
- src/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_store_request.py
- src/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_match.py
- src/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_replay_result.py
- src/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_replay_input.py
- src/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_learn_result.py
- src/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_learn_command.py
- src/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_entry.py
- tests/unit/nodes/node_chain_learning/test_models.py
- src/omnibase_infra/nodes/node_chain_orchestrator/models/model_chain_retrieval_result.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/omnibase_infra/nodes/node_chain_orchestrator/handlers/handler_chain_store_complete.py
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.
Summary
Architecture
Test plan
uv run pytest tests/unit/nodes/node_chain_learning/ -v)Summary by CodeRabbit