feat(resource): add resource type inheritance and polymorphic tool matching #618
Assignees
aditya (Aditya Chhabra)
aleenaumair (Aleena Umair)
brent.edwards (Brent Edwards)
CoreRasurae (Luis Mendes)
drew (Drew Morris)
eugen.thaci (Eugen Thaci)
freemo (Jeffrey Phillips Freeman)
HAL9000 (HAL 9000)
HAL9001 (HAL9001)
hamza.khyari (Hamza Khyari)
hurui200320 (Rui Hu)
justin.morris
khird (Kyle Hird)
org.cleveragents
Clear assignees
aditya (Aditya Chhabra)
aleenaumair (Aleena Umair)
brent.edwards (Brent Edwards)
CoreRasurae (Luis Mendes)
drew (Drew Morris)
eugen.thaci (Eugen Thaci)
freemo (Jeffrey Phillips Freeman)
HAL9000 (HAL 9000)
HAL9001 (HAL9001)
hamza.khyari (Hamza Khyari)
hurui200320 (Rui Hu)
justin.morris
khird (Kyle Hird)
org.cleveragents
No Assignees
hamza.khyari
hamza.khyari
Notifications
Due Date
No due date set.
Blocks
#513 feat(resource): add resource type inheritance and polymorphic tool matching
cleveragents/cleveragents-core
Reference: cleveragents/cleveragents-core#618
Reference in New Issue
Block a user
Delete Branch "feature/m6-resource-type-inheritance"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Implements ADR-042 single-inheritance for resource types with polymorphic tool and handler resolution.
Closes #513
Changes
Core
inheritsfield toResourceTypeConfigSchema,ResourceTypeSpec, andResourceTypeModelinheritance.pymodule:resolve_inheritance_chain(),validate_chain(),is_subtype_of(),resolve_fields(),find_subtypes()ResourceRegistryService: chain validation onregister_type(),resolve_type_inheritance_chain(),is_subtype_of()find_tools_for_resource()toToolRegistrywith polymorphic matchingresolve_handler_polymorphic()to handler resolverm6_004_resource_type_inheritsaddsinheritscolumn + indexCLI
agents resource type listshows Inherits columnagents resource type showdisplays full inheritance chainTests
resource_type_inheritance.feature(all pass)Docs
docs/reference/resource_type_inheritance.md(full reference)docs/schema/resource_type.schema.yamlQuality
ADR-042 Compliance
ISSUES CLOSED: #513
cb44db497fto87cdd1b2e9PM Status Check — PR #618
Author: @hamza.khyari | Milestone: v3.5.0 (M6) | Mergeable: No (conflict) | Reviews: None
Issues
Positive Notes
Action Items
87cdd1b2e9tod3d076ebab@hamza.khyari This PR has a merge conflict. Please rebase onto master. Branch naming (
feature/) is correct for feature work.PM Status Check — Day 29
Author: @hamza.khyari | Milestone: v3.5.0 (M6) | Mergeable: NO (conflict) | Reviews: None
Current State
Resource type inheritance and polymorphic tool matching (issue #513, ADR-042). 30 BDD scenarios, 18 Robot tests, quality gates pass per PR description. However:
Action Required
Priority note: @hamza.khyari — You have 4 open PRs (#610, #612, #615, #618) with merge conflicts and PR #610 has 20 P1 findings awaiting response. Please triage: PR #610 first, then rebase the others in dependency order.
a67aee6b0bto3815d38646d70f57edc6to7ddf5f5543PM Status Check — Day 29
Author: @hamza.khyari | Milestone: v3.6.0 (Post-MVP) | Reviews: None
Current State
Resource type inheritance implementation. Has a merge conflict — needs rebase. No reviewer assigned.
Action Required
Lower urgency — Post-MVP milestone. Same guidance as PR #615: M5 work takes priority.
PM Compliance Audit — PR #618
Auditor: PM (automated sweep) | Date: 2026-03-09 | Reference: CONTRIBUTING.md §Pull Request Process (items 1–12)
Checklist
Closes #N/Fixes #N)Closes #513in body +ISSUES CLOSED: #513in footerType/FeaturepresentReview Status
Merge Readiness
NOT READY — Blockers:
Action items for @hamza.khyari:
Note to reviewers: This is an M6 feature implementing ADR-042 (resource type inheritance). 30 BDD scenarios, 18 Robot tests, 14 ASV benchmarks. Quality gates reported passing by author. Priority: Medium — needed for M6 completion but not on critical path.
ADR-042 compliance note: Author self-reports full compliance (single inheritance, max chain depth 5, no circular inheritance, built-in guard, subtype deletion guard). Reviewers should verify these constraints in the implementation.
PM Escalation — Merge Conflict + No Reviews (Day 29)
@hamza.khyari This PR has had a merge conflict since @freemo flagged it on March 7 and has received zero code reviews in 3 days.
Required actions:
This is an M6 feature (resource type inheritance / ADR-042). While not on the critical path, it's needed for M6 completion. Please rebase and request reviews.
Code Review — PR #618 — Round 1
Reviewer: @brent.edwards | Method: 5-pass lens protocol + adversarial re-read | Branch:
feature/m6-resource-type-inheritance@7ddf5f5Nox Verification Matrix
typechecklintunit_testsintegration_testscoverage_reportIntegration test note: The full
nox -e integration_testssession timed out after 10 minutes. This may be a CI infrastructure issue rather than a code bug — the unit tests (which cover BDD + parallel behave) all pass. Author should confirm integration tests pass in their local environment.File Size Compliance (CONTRIBUTING.md 500-line limit)
inheritance.pyresource/schema.pyhandlers/resolver.py_resource_registry_data.py_resource_registry_ops.pyresource_registry_service.pytool/registry.pycli/commands/resource.pyArchitecture Review Checklist
inheritance.py(engine),schema.py(validation),resolver.py(handler loading)Anywithout justification;TypeRegistryMap = dict[str, Any]is appropriate since registry entries can be dicts or objectsmodels.pyonly adds columnResourceTypeCircularInheritanceError, etc.) all inherit fromValueErrorinherits=NULLfor existing types;_spec_to_db/_db_to_specbackward-compat aliases providedCLI Review Checklist
type listandtype showenhanced--format jsonoutput is parseable — ⚠️ Not verified;_resource_type_dictaddsinheritskey but JSON format path not testedResourceTypeParentRemovalErrordisplayed cleanlyDB Migration Review Checklist
upgrade()anddowngrade()functionsdowngrade()is actually tested — ⚠️ No downgrade test in BDD/Robotadd_column+create_indexonlyIF NOT EXISTSguard — ⚠️op.create_indexwithout guardm6_003 → m6_004Findings
All findings grouped by file, sorted by severity.
src/cleveragents/application/services/resource_registry_service.py[P2]
resource_registry_service.py:345-419— Diff coverage at 95%, below 97% threshold. 11 lines uncovered (lines 345-347, 385-387, 415-419). The exception handlers inregister_type()andremove_type()lack test coverage. These are the rollback/re-raise paths for unexpected exceptions.src/cleveragents/application/services/_resource_registry_data.py[P2]
_resource_registry_data.py:BUILTIN_TYPES— Ordering dependency is implicit.devcontainer-instance(index 3) inherits fromcontainer-instance(index 2). If someone reorders the list,bootstrap_builtin_types()fails withResourceTypeParentNotFoundError. Add a comment at the list head:# IMPORTANT: Order matters — children must appear after their parents.[P3]
_resource_registry_data.py:get_ancestors()— Usesqueue.pop(0)which is O(n) per dequeue. Usecollections.dequewith.popleft()for O(1).src/cleveragents/application/services/_resource_registry_ops.py[P2]
_resource_registry_ops.py— Mixin classes use# type: ignore[attr-defined]to access host class methods (self._session(),self._load_type_registry(),self.show_resource()). This circumvents Pyright's ability to verify the contract. Define a protocol or ABC that declares the required interface so the mixin's dependency is explicit and type-safe.[P3]
_resource_registry_ops.py— At 496 lines, this file is 4 lines from the 500-line CONTRIBUTING.md limit. Any additions will push it over. Consider a proactive split (e.g., extractResourceDagMixinto its own file).src/cleveragents/resource/inheritance.py[P2]
inheritance.py:_COLLECTION_FIELDS— Includes"properties"butResourceTypeSpechas nopropertiesfield.resolve_fields()with properties merging only works for raw dict registry entries (e.g., in-memory test registries), not for domain model entries that go throughmodel_dump(). This inconsistency could cause silent field loss when the registry containsResourceTypeSpecobjects. Either addpropertiestoResourceTypeSpecor remove it from_COLLECTION_FIELDSwith a comment explaining the limitation.[P3]
inheritance.py:_to_dict()— Thevars(entry)fallback for non-dict, non-Pydantic entries may include private attributes (prefixed with_) in field resolution results. Consider filtering out keys starting with_.src/cleveragents/domain/models/core/resource_type.py[P2]
resource_type.py:ResourceTypeSpec.inherits— Missingfield_validator("inherits")for name format validation. OnlyResourceTypeConfigSchemavalidates the inherits name pattern (_BUILTIN_NAME_RE/_NAMESPACED_RE). Direct construction viaResourceTypeSpec(inherits="!!!!")orfrom_config({"inherits": "!!!!"})bypasses format validation. Add the same validator fromResourceTypeConfigSchema.alembic/versions/m6_004_resource_type_inherits.py[P2]
m6_004_resource_type_inherits.py:upgrade()— NoIF NOT EXISTSguard onop.create_index. If the migration is re-run (e.g., during development), the index creation will fail. Consider using Alembic'sif_not_exists=Trueparameter.[P3]
m6_004_resource_type_inherits.py— No downgrade test. While the downgrade function is simple (drop index + drop column), it's not exercised by any test. Consider adding a Robot or BDD scenario that runs upgrade→downgrade→upgrade.features/steps/resource_inheritance_coverage_steps.py[P3]
resource_inheritance_coverage_steps.py— Directly imports private_handler_cachefromresolver.pyinstead of using the publicclear_handler_cache()API. While this works, it couples the test to an internal implementation detail.Summary
Verdict
This is a well-crafted, architecturally sound PR. The inheritance engine in
inheritance.pyis clean, well-documented, and thoroughly tested (52 BDD scenarios + 18 Robot tests + 14 ASV benchmarks). The refactoring ofresource_registry_service.pyinto three modules to stay under the 500-line limit is commendable. The CLI enhancements, migration, and documentation are all present and thorough.No P0 or P1 blockers found. The six P2 findings are "should-fix" items that can be addressed in a follow-up PR within 3 days per the review playbook. The most important P2s are:
field_validator("inherits")onResourceTypeSpec(validation gap)_COLLECTION_FIELDS/propertiesinconsistency (silent field loss risk)resource_registry_service.py(95% < 97% threshold)PM blocker: The PR has a merge conflict that must be resolved before merge.
Approved with Comments
Detailed findings in comment #57137.
0 P0 | 0 P1 | 6 P2 | 5 P3
No blockers. This is architecturally sound, well-tested (52 BDD + 18 Robot + 14 ASV), and the refactoring of
resource_registry_service.pyinto three sub-500-line modules is well done.The 6 P2 findings are "should-fix" items per the review playbook — address in a follow-up PR within 3 days:
field_validator("inherits")onResourceTypeSpec_COLLECTION_FIELDSincludes"properties"butResourceTypeSpeclacks the fieldresource_registry_service.pydiff coverage at 95% (< 97% threshold)BUILTIN_TYPESordering dependency is implicit (needs comment)type: ignore[attr-defined]should be replaced with a protocolop.create_indexlacksif_not_existsguardMerge blocker: Resolve the merge conflict with master first.
Reviewer Response — @brent.edwards
Responding to PM compliance audit (#57071), PM escalation (#57088), and PM status checks (#56533, #56808).
Review Status
The PM audit (#57071) and escalation (#57088) both noted "No formal code reviews yet." This has been resolved:
code_review_protocol.md(5-pass lens) andreview_playbook.md(Architecture + CLI + DB Migration checklists).Findings Summary
0 P0, 0 P1, 6 P2, 5 P3 — No merge blockers from my review. APPROVED.
The most important P2 items for @hamza.khyari to address (can be in a follow-up PR per review playbook):
field_validator("inherits")onResourceTypeSpec— validation gap_COLLECTION_FIELDSincludes"properties"butResourceTypeSpechas nopropertiesfield — silent field loss riskresource_registry_service.pydiff coverage at 95% (below 97%)op.create_indexlacksif_not_existsguard in migration# type: ignore[attr-defined]— should define a protocolBUILTIN_TYPESordering dependency is implicit — needs commentPM Action Items
ADR-042 Compliance Verification
Per the PM's note to verify ADR-042 constraints:
ResourceTypeSpec.inherits: str | None(single parent)_walk_chain()withMAX_CHAIN_DEPTH = 5_walk_chain()tracksvisitedset and raisesResourceTypeCircularInheritanceErroris_builtin()check prevents modification of built-in typesremove_type()checks for children before deletion, raisesResourceTypeParentRemovalErrorAll ADR-042 constraints verified in the implementation.
7ddf5f5543toab4da7804dNew commits pushed, approval review dismissed automatically according to repository settings
- P2-1: Add field_validator('inherits') on ResourceTypeSpec - P2-2: Remove 'properties' from _COLLECTION_FIELDS (scalar override) - P2-3: Add behave scenarios for exception rollback paths - P2-4: Add ordering comment to BUILTIN_TYPES - P2-5: Define RegistryHost Protocol, remove type:ignore on mixins - P2-6: Add if_not_exists=True on migration index creation - P3-1: Use deque.popleft() in get_ancestors for O(1) BFS - P3-2: Filter private attrs in _to_dict vars() fallback - P3-3: Replace _handler_cache import with clear_handler_cache() - P3-4: Add migration downgrade test for m6_004 - P3-5: Extract ResourceDagMixin to _resource_registry_dag.py All files remain under 500-line CONTRIBUTING limit. Lint, typecheck, and 85 behave scenarios pass.