refactor(architecture): simplify extension authoring and correct pipeline contracts - #167
Merged
Merged
Conversation
Settle the three pending decisions (stage-based error mapping with -100 for execution failures, shape inference for port cardinality, whole-batch rollback kept) and track each adopted item for incremental delivery. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rdinality strings Input cardinality now states how a node consumes items and accepts any upstream shape, so split outputs can feed item-wise nodes such as TextRuleMatchNode. The Validator derives each key's per-request shape along the DAG and reports PORT_CARDINALITY_MISMATCH only when a 1:1 biz or IO boundary receives split items, or when item-wise inputs of one node cannot pair item by item. TextEmbeddingNode declares its real 1:1 relation instead of the N:M bypass. Provenance and lifetime checks are unchanged; Pipeline JSON and Definition fields keep their format. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TextRuleMatchNode now publishes a neutral RuleMatchItem: scalar fields for the first or default hit, every hit in matches, and typed slots for captures, constants and the default raw_query. The keyword and audio converters share an Adapter-private serializer, so renaming a JSON key no longer happens inside a common Node. A byte-level Operator contract test written against the previous implementation keeps multi-rule hits, typed slots and the default hit unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…odes Internal Pipeline, Node and Model codes no longer reach the host. Create preparation failures (configuration, deployment, model or backend loading) return -2 while unsupported biz and registry conflicts keep -5 and -6; Node or Model execution failures in Process return -100; Control returns -2 for rejected requests, -7 for unsupported commands and -100 when a Node rejects or fails to apply an update. Pipeline::Control now reports the failure stage so the Adapter does not classify by integer. GetOperatorLastError() keeps the stage, internal code and node detail. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…sults Fatal errors keep whole-batch atomicity. Each existing biz contract now states what its output converter writes to status_code and which invalid single results (failed or fallback documents, out-of-enum risk levels, invalid rankings) reject the batch, so partial success remains a per-contract design decision rather than a framework channel. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Six structs in include/adapter/biz_results.h had no type references and converters never used the header. The only consumer, the nested output allocator fixture, now owns a minimal NestedOutputSource, so the header is deleted and onboarding points at the neutral result contracts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
InputPortBindings and OutputPortBindings duplicated the same lookup and storage. Both are now aliases of PortBindings<PortDirection>, which keeps them distinct, non-convertible types so decoders and encoders still receive only their own direction. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t config ResolvedInputLimits was never configured: the resolver always stored its defaults and the handle copied them per call. It is now InputLimits, the host-memory safety bounds that value types check before reading, passed as defaults by the facade and still injectable by tests and authored value types. Buffer and Any carrier bounds become named biz_input constants, so every limit has one definition. The converter checks stay as the business-semantics layer; read ranges and error order are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…stration RuntimeOptions::biz_type, depth_num and biz_name were written by the Adapter but never read; Pipeline only forwards the device and chip target. ModelManager::RegisterModel was a test convenience over the atomic batch path that production uses, so tests and the authoring benchmark now register through RegisterTestModel or RegisterBatch. Read accessors, UpdateModelRevision and SetResource/GetResource stay because tests rely on them to prove atomic registration, revision-keyed caching and resource type checks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
BindPort, BindPorts, BindInput, BindOutput, Require and Publish had no runtime callers: AuthorNode binds through its Spec bindings. They now live in dev_support's LegacyNodeBase, used by tests that drive the NodeBase runtime directly and by the authoring benchmark, whose historical ModelBoundNode baseline is rewritten to inherit it. NodeBase keeps only the lifecycle and exception barriers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
AuthorNode<MapSpec> duplicated port binding, parameter parsing, snapshots and Control dispatch. It now converts the MapSpec into a Batch runtime spec with one required anchor input, one preserved output and a per-item loop that names the node and failing item; the Definition still comes from MapSpec, so the author API and Catalog are unchanged. Batch binding diagnostics now report expected and bound types like Map did. A new test, passing on both implementations, pins the item failure, empty-message fallback and empty-batch behavior. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Each Node instance evaluates its Spec afresh, models and parameters already live on AuthorNode, and the only per-instance binding state is the resolved Blackboard key, so splitting declarations would add a parallel key table without removing type erasure or copies. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The five capability calls repeated the same model ownership, move-only rule, slot identity and accessors. They now derive from detail::ModelCallBase and keep only their named capability method (Generate, Embed, Transcribe, Recognize, Score). A contract test pins the shared move-only semantics and default slot names. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Validator populated Chinese remediation summaries, candidate facts and JSON Patch fixes inside Core, so every SDK Create failure ran tooling code that only alg_pipeline_tool and Studio consume. That code now lives in src/cli/pipeline_remediation as the edgeflow_pipeline_tooling library; fix verification still re-runs PipelineValidator, so no rule is reimplemented. Nearest registered names for unknown node, model and backend types stay in Core suggestions. PipelineValidator::Explain is removed. validate and --explain output for 15 broken documents is byte-identical before and after the move. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Init-time binding audit already checks converter and biz keys and types; the Validator's remaining cross-checks cover flow contracts and converter-required optional outputs. Moving biz ownership to the Adapter would touch Catalog, CLI/Studio and biz_name without a driving defect. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Control is a low-frequency update path, Core's broadcast precheck and a Node's direct-call validation have distinct duties, and reusing the parsed payload would change INode::Control for every node and test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… payloads Control payload validation re-implemented the type, enum, minimum and maximum keyword checks already provided by json_structure. It now calls those predicates and keeps its own messages and error contract. Config field validation stays separate because its typed definitions, default filling and diagnostic paths differ. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PreparedDeployment and ValidatedIoPlan repeated the same eight fields, copied one by one and partly renamed. Both now derive from IoBindingSelection, so the validated plan moves the prepared selection in one step and output pool fields keep one name. The Operator handle reads converters from the runtime's plan instead of storing copies, and ResolvedOperatorConfig no longer duplicates the binding identity. Preparation and resource loading remain separate stages. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every production converter hand-wrote external_type as its slot types joined in declaration order. The registry now fills that value when the field is empty and keeps explicit protocol labels, so 16 converter definitions drop the duplicated string; the generated Catalog is byte-identical. Protocol identity, schema version, slot value types and capacity policy checks are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Host structs are platform definitions shared across converters, so each type needs one owner for its traits and layout; splitting registration per type adds files without removing steps, and real SDK types can only be integrated inside the authorized network. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every Model repeated its type, capability and concurrency in class constants, three IModel overrides and its ModelDefinition. Models now derive from ModelIdentity<Model, CapabilityInterface>, declare only kModelType and kConcurrency, take the capability from ModelCapabilityTraits, and start their Definition from MakeModelDefinition<Model>(). The runtime factory still checks each created instance against its Definition; the Catalog is byte-identical. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The four production Backends repeated their type name in kBackendType, a BackendType() override and the BackendDefinition. Providers now derive from BackendIdentity<Backend> and start their Definition from MakeBackendDefinition<Backend>(); the factory still checks provider and session types. Function-based provider registration is not adopted: barriers, diagnostics, session ownership and protocol checks live in Load and the factory either way, while the registry API and every test backend would change. The Catalog is byte-identical. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tions Models and Backends declared each config default in their Definition and repeated it as the fallback of json::value() at creation (33 reads, all matching today). They now read through ConfigValueOrDefault, a neutral contracts helper that falls back to the declared default, and expose their Definitions through accessors so creation code can reach them. Factory normalization, semantic validation and creation checks are unchanged; the Catalog is byte-identical. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Vendor headers are already confined to guarded blocks, neutral helpers are separate and tested, and moving the split into CMake would require extra gating plus an on/off build matrix for four switches. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The IoConverter, IoBinding, Model and Backend registries each kept their own conflict flag and message list. They now hold a contracts-level RegistryConflicts guarded by their existing mutex. The Adapter registries previously derived HasConflict from a non-empty message list, so a message lost to an allocation failure reopened the registry; the shared state keeps the conflict and reports a fallback message, covered by an allocation-failure test. NodeRegistry keeps its atomic flag for lock-free reads during registration callbacks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fourteen of eighteen include/core headers are already node-visible contracts and the manifest-driven header views, probes and guards enforce the boundary; moving files would rewrite many includes and the layer documentation without changing a dependency. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
List the source-extension changes made while adopting the architecture review and mark every progress item as completed or declined with its reason; the plan is deleted when its last stage merges. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
chamsechan
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4430642696
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+94
to
+95
| std::vector<RuleMatchHit> matches; | ||
| nlohmann::json slots = nlohmann::json::object(); |
There was a problem hiding this comment.
Running dev_support/benchmarks/control_snapshots.py now fails during compilation: its shared benchmark source still reads RuleMatchItem::captures at control_snapshots.cpp:116, and the historical text_rule_match_node.cpp compiled at lines 113–118 also constructs and populates the removed captures, constants, details, and match_result_json members against this current header. Update the benchmark and provide a compatibility transformation/type for the historical source so the rules comparison remains runnable.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pipeline validation rejected valid split-to-item-wise graphs, internal failure codes collided with public Operator codes, and extension authors had to repeat identity and configuration defaults. This branch corrects those contracts and consolidates the duplicated authoring and registration code.
The Operator ABI, host carriers, deployment conf, and Pipeline JSON format are retained. Public failure-return behavior intentionally changes: creation preparation failures use -2, node/model execution failures use -100, and invalid Control requests use -2; GetOperatorLastError retains the internal cause and stage. Source-extension API changes are listed in doc/CHANGELOG.md.
Validation: 176 focused tests passed, including move-only callback ownership, controls and failures; inferred shapes and IO-boundary facts; and configuration guards. The repository delivery script passed the canonical default gate (103/103 CTest entries) before committing and pushing. Independent reviews cover the affected Node ownership and Core/CLI contracts. PR CI and the main push CI for the exact merge SHA are required by the delivery workflow.