feat(resource): add deferred virtual resource types #663
No reviewers
Labels
No labels
auto/needs-reevaluation
controller-managed
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
No project
No assignees
4 participants
Notifications
Due date
No due date set.
Blocks
#331 feat(resource): add deferred virtual resource types
cleveragents/cleveragents-core
Reference
cleveragents/cleveragents-core!663
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature/post-resource-types-virtual"
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 Forgejo issue #331 -- adds 3 deferred virtual resource types (
remote,submodule,symlink) with equivalence metadata rules to the resource registry.Depends on: #662 (deferred physical types) -- the
child_typesreferenced by these virtual types (git-remote,git-submodule,fs-symlink,git-tree-entry) are introduced by #662. This PR must merge after #662.Recommended merge order: #662 → #661 → #663
Changes
Type Definitions (
_resource_registry_virtual_deferred.py)remote-- equivalence criteria:["url"]; children:git-remotesubmodule-- equivalence criteria:["url", "path"]; children:git-submodulesymlink-- equivalence criteria:["name", "target_path"]; children:fs-symlink,git-tree-entryresource_kind: virtual,user_addable: false,sandbox_strategy: none,built_in: true,handler: None, all capabilities falseYAML Configs (
examples/resource-types/)remote.yaml,submodule.yaml,symlink.yamlwith spec reference commentsModel Validation (
resource_type.py)criteriamust be a non-emptylist[str]with each element a non-empty stringuser_addablemust befalse,sandbox_strategymust benonehandlermust beNone, all capabilities must befalseRuntime Guard
register_resource()guard at_resource_registry_ops.py:112-116rejects manual creation of types withuser_addable=Falseremote,submodule,symlinkTests
Documentation
docs/reference/resource_types_builtin.md-- deferred virtual types section with equivalence metadata tablesEquivalence Metadata (Critical for #334)
remote["url"]submodule["url", "path"]symlink["name", "target_path"]Spec Reference
docs/specification.md~line 23705 (virtual types table), lines 24619-24631 (identity key table)Closes #331
8462326710cc1b016befcc1b016bef3e4c2e8286PM Status (Day 31):
Merge conflict detected. The 17 PR merges from Days 30-31 (including your own 5 merges) significantly updated
develop.Action required: @hamza.khyari — please rebase this PR against current
developand push.Priority: M6 work — continue at pace. Your Days 30-31 velocity was excellent (5 merges).
PM Review — Day 31 (Specification Update)
Merge conflict detected. This conflict is due to significant specification changes made today (ACP→A2A protocol adoption, TUI overhaul).
Spec Alignment Check
Resource type work is NOT impacted by the protocol or TUI changes. Resource models are in the domain layer, orthogonal to the transport layer.
Milestone
Remains v3.6.0 (M7 — Advanced Concepts). Resource types are explicitly M7 scope.
Action Required
@hamza.khyari — Rebase against
master. Priority: After v3.5.0 UKO work (#657, #660).PM Status — Day 32
Status: CONFLICTED — needs rebase. Part of resource type series (#661-#664).
PR: Deferred virtual resource types (remote, submodule, symlink). M7 (v3.6.0), due Mar 28. Author: @hamza.khyari. 42 BDD scenarios, 100% diff coverage.
Action Required: @hamza.khyari — Rebase after #662 merges. Labels incomplete — missing MoSCoW, Points.
Rebase Required
@hamza.khyari — This PR has merge conflicts with
master. Please rebase onto the latestmasterand force-push. See also: #661, #662, #664 (all need rebase).PM Day 36: Deferred virtual resource types. Closes #331. M7 scope. Sequential dependency on #662. @hamza.khyari author.
3e4c2e82861a7673cb921a7673cb92ff1c1ed6f6ff1c1ed6f6a73307e0aaPR #663 Review — Deferred Virtual Resource Types
Summary
Adds 3 deferred virtual types (remote, submodule, symlink) with equivalence metadata. Smallest of the three PRs (1,365 lines, 13 files). Also enhances virtual type validation with 5 new constraint checks. The validation improvement is a valuable contribution. However, this PR has hard dependencies on both #661 and #662.
P0 — Must Fix
1. Hard dependency on PR #662 — all
child_typesreference types from #662remote→child_types: ["git-remote"]— introduced by #662submodule→child_types: ["git-submodule"]— introduced by #662symlink→child_types: ["fs-symlink", "git-tree-entry"]— both introduced by #662None of these child types exist on master. This PR cannot merge before #662 without leaving dangling child_type references. This needs to be documented as a formal dependency (e.g., "Depends on #662" in the PR description).
2. Three-way merge conflict on
BUILTIN_NAMESThis PR appends 3 names at the end of the existing frozenset. PR #662 rewrites the entire block with sorting. Guaranteed conflict. Must rebase onto #662.
3. Three-way merge conflict on
_resource_registry_data.pyThis PR inlines 3 type dicts after
*DATABASE_TYPE_DEFS. PR #661 adds*VIRTUAL_CORE_TYPESat the same location. PR #662 changes the splice point for*DATABASE_TYPE_DEFS. All three conflict.P1 — Should Fix
4. Enhanced virtual validation belongs in an earlier PR
The expanded virtual type validation in
resource_type.py(checkingcriteria,user_addable,sandbox_strategy,handler,capabilities) is excellent — it codifies invariants that all virtual types must satisfy. However, this validation would benefit #661's virtual core types too. If #661 merges before #663, the 6 core virtual types won't have these checks until #663 lands.Suggestion: Move the validation enhancement to #661 (or a prerequisite PR) so all virtual types are validated from the start.
5. Inconsistent extraction pattern — types inlined instead of extracted
PRs #661 and #662 extract their type definitions to separate modules (
_resource_registry_virtual.py,_resource_registry_physical.py). This PR inlines the 3 type dicts directly in_resource_registry_data.py, pushing it to 480 lines (20 lines from the limit). While 3 types is fewer than 6 or 11, the inconsistency means_resource_registry_data.pywill be the bottleneck for future additions.Suggestion: Extract to
_resource_registry_virtual_deferred.pyfor consistency, or add the 3 types to #661's_resource_registry_virtual.pyif merge order permits.6. Drive-by docstring cleanup
Removing the
implementation_plan.mdreference fromresource_registry_service.pyis fine but should be noted in the PR description as an incidental cleanup. It changes lines that could also be touched by #661 and #662 (both modify the same docstring table), adding to the conflict surface.P2 — Nice to Have
7. Equivalence metadata uses
"criteria"key namingPR #661 uses
"criteria"in its equivalence dicts. The PR description here usesidentity_keyandidentity_fieldsterminology. The actual code uses"criteria". The naming should be consistent with whatever the equivalence tracking service (#334) will expect. If #334 expectsidentity_key/identity_fields, the data structures here need to match.8. Validation could use structural typing
This runtime validation is correct but could be replaced with a nested Pydantic model (e.g.,
EquivalenceSpec) for compile-time safety. Not a blocker — matches the currentdict[str, Any]pattern.P3 — Observations
resource_kind: virtual,sandbox_strategy: none,user_addable: false, capabilities all false,handler: null/None.urlfor remote,url+pathfor submodule,name+target_pathfor symlink.feat(resource): add deferred virtual resource types— correct.resource_type.pyat 476 lines — comfortable margin.Verdict: Request Changes
P0: Hard dependency on #662 for child_type references. Must be documented and merge order enforced: #662 → #661 → #663.
Recommended Merge Order (Cross-PR)
After each merge, the next PR must rebase to resolve conflicts in
_resource_registry_data.py,resource_type.py, andresource_registry_service.py.@ -196,6 +196,92 @@ BUILTIN_TYPES: list[dict[str, Any]] = [},},*DATABASE_TYPE_DEFS,# === Deferred Virtual Resource Types (#331) ===P1: Unlike #661 (
_resource_registry_virtual.py) and #662 (_resource_registry_physical.py), these 3 type dicts are inlined here, pushing the file to 480 lines. Consider extracting to_resource_registry_virtual_deferred.pyfor consistency with the sibling PRs.@ -199,0 +207,4 @@"sandbox_strategy": "none","user_addable": False,"built_in": True,"cli_args": [],P0: All three child_types here (
git-remote,git-submodule,fs-symlink,git-tree-entry) are introduced by PR #662. This PR cannot merge before #662. AddDepends on #662to the PR description.@ -310,3 +311,1 @@f"Virtual resource type '{self.name}' requires an ""'equivalence' configuration for deduplication.")# Virtual types require equivalence rules with criteria and haveP1: This enhanced virtual validation (criteria check, user_addable, sandbox_strategy, handler, capabilities) is excellent but should ideally land before #661's virtual core types, so all virtual types get these checks from the start. Consider moving this validation to #661 or a shared prerequisite PR.
Code Review — PR #663
feat(resource): add deferred virtual resource typesReviewer: @brent.edwards | Size: M (116 new lines) | Focus: Equivalence metadata, type taxonomy
P1:must-fix (2)
1.
child_typesreference types that only exist after #662 mergessymlinkdeclareschild_types: ["fs-symlink", "git-tree-entry"]. Both types are introduced by PR #662.remote→git-remoteandsubmodule→git-submodulehave the same dependency. If #663 merges before #662, bootstrap registration fails or creates types with dangling child references.Fix: Add
Depends on: #662to PR description. Merge order: #662 → #661 → #663.2. Constructor auto-bootstrap removed without migration path
The PR removes automatic
bootstrap_builtin_types()from the service constructor — callers must now call it explicitly. This is a behavioral change that affects ALL existing code paths that rely on implicit bootstrap (CLI commands, container initialization, tests). If any existing caller doesn't callbootstrap_builtin_types(), types silently won't exist.Fix: Document which callers need updating. Add a deprecation warning for 1 release before removing auto-bootstrap, or verify ALL callers have been updated.
P2:should-fix (1)
3.
symlinktype declareschild_types: ["fs-symlink", "git-tree-entry"]— butgit-tree-entryis a generic blob/subtree entry, not specific to symlinks. This means a non-symlink tree entry (e.g., a regular file entry) could be incorrectly linked to the virtualsymlinktype. The equivalence tracking (#664) needs to verify the tree entry's mode (symlink mode=120000) before creating the link.P3:nit (1)
4.
resource_type.pyat 486 lines — comfortable margin for now.Positive Observations
Verdict: REQUEST_CHANGES — merge dependency (P1-1) and auto-bootstrap removal impact (P1-2) need resolution.
a73307e0aa8a016a48f8Response to Review Findings
Thanks for the thorough review. Addressed all actionable items:
P0-1: Hard dependency on #662 (child_types) — ADDRESSED
Documented in PR description: "Depends on: #662" with recommended merge order
#662 → #661 → #663. Also documented in the commit message body. Note:child_typesis a declarativelist[str], not a FK constraint — bootstrap does not validate that child types exist as registered types, so no runtime failure if merged before #662, but the references would be unresolvable until #662 lands. Agreed this should merge after #662.P0-2 / P1-2: Constructor auto-bootstrap removed — INCORRECT
ResourceRegistryService.__init__still callsself.bootstrap_builtin_types()at line 105. This was NOT removed by this PR. The stale PR description that claimed "Constructor auto-bootstrap removed" has been corrected. The current PR description now states: "Auto-bootstrap inResourceRegistryService.__init__is preserved (NOT removed)".P1-4: Enhanced validation belongs in earlier PR — NOTED
Valid suggestion for merge strategy. The validation (criteria, user_addable, sandbox_strategy, handler, capabilities checks) benefits all virtual types. If #661 merges first, we can cherry-pick or forward-port the validation block. For now it lives here since this PR introduced the checks.
P1-5: Inconsistent extraction pattern — ADDRESSED
Extracted the 3 type dicts to
_resource_registry_virtual_deferred.py(115 lines), imported and spread as*VIRTUAL_DEFERRED_TYPESin_resource_registry_data.py. This matches the pattern from #661 (_resource_registry_virtual.py/VIRTUAL_CORE_TYPES)._resource_registry_data.pyis now 399 lines (down from 480).P1-6: Drive-by docstring cleanup — NOTED
The
implementation_plan.mdreference removal is incidental. Noted in the changes as a cleanup.P2-3: git-tree-entry not symlink-specific — ACKNOWLEDGED
Correct observation. The equivalence tracking service (#664) must filter
git-tree-entryby mode=120000 (symlink mode) before creating links. This is a #664 concern, not a type definition issue — thechild_typesdeclaration says "these physical types can be children", not "all instances of these types are children".P3-7: criteria naming — ALREADY RESOLVED
Code uses
"criteria"per spec. The old PR description referencedidentity_key/identity_fieldswhich has been corrected.P3-8: Structural typing for equivalence — NOTED
Agreed a nested
EquivalenceSpecPydantic model would be better thandict[str, Any]. Out of scope for this PR — could be a follow-up.Current state: 47 Behave scenarios, 7 Robot tests, lint clean, typecheck 0 errors,
8a016a48.Code Review — PR #663
REQUEST_CHANGESThis PR adds the deferred virtual resource types and related docs/tests, but two merge-critical gaps remain: (1) runtime behavior does not enforce
user_addable: false, so users can still manually createremote/submodule/symlinkresources; and (2) on currentmasterthese new virtual types reference child types that are not yet registered, so the PR remains sequencing-dependent on the physical-type PR. There are also test/metadata mismatches that create false confidence and should be corrected before merge.Findings Table
src/cleveragents/application/services/_resource_registry_ops.pyregister_resource()persists any existing type without checkingResourceTypeModel.user_addable. Runtime repro on this branch source (PYTHONPATH=src):register_resource(type_name='remote'/'submodule'/'symlink', ...)succeeds (ADDED ...).user_addablein service/CLI path (prefer service-level guard): reject manual creation whentype_row.user_addable is Falseunless call path is internal auto-discovery. Add explicit negative tests for these three types via service and CLI.Codemaster; PR still requires strict dependency sequencing.remote -> ['git-remote'] missing ['git-remote'],submodule -> ['git-submodule'] missing ['git-submodule'],symlink -> ['fs-symlink','git-tree-entry'] missing ['fs-symlink','git-tree-entry'].PR/Processfeatures/resource_type_deferred_virtual.featureandrobot/resource_type_deferred_virtual.robotinclude no scenario/case assertingregister_resource('remote'...)fails. Existing tests all pass while runtime allows add.Codesrc/cleveragents/domain/models/core/resource_type.py: checks onlycriteriaexists and is non-empty list. Repro:ResourceTypeSpec.from_config(... equivalence={'criteria':[123]})and['']both pass.criteriato be a non-emptylist[str]with each entry non-empty and trimmed; add negative tests for invalid item types/values.Code_resource_registry_virtual_deferred.py; current branch has one robot file with 6 test cases and deferred type dicts are inlined in_resource_registry_data.py.PR/ProcessConsolidated Checklist
remote,submodule,symlink) in runtime path.equivalence.criteriavalidation tolist[str]with non-empty entries.Fix Plan
register_resource()to rejectuser_addable=Falsetypes for manual calls.remote/submodule/symlink.ResourceTypeSpeccriteria item validation and add negative tests for non-string/empty entries.Dependencies:
PM Status — Day 37 (2026-03-17)
Status: IN REVIEW — Hamza responded to review findings (Day 37). Aditya posted REQUEST_CHANGES with 5 findings (2 P1, 2 P2, 1 P3). Key blockers: runtime doesn't enforce
user_addable: false(F-001) and child type dependency on #662 (F-002).Blockers: P1 findings unresolved. Depends on #662 merge order.
Action required:
user_addableguard) and F-003 (negative tests). Push fix commit.PM status — Day 37
Re-Review (Round 2) — PR #663
Checked the force-pushed update (
8a016a48) against the 4 findings from Round 1.Resolved Findings
P1-1 (child_types reference types from #662): RESOLVED
The PR body now explicitly states: "Depends on: #662 (deferred physical types) — the
child_typesreferenced by these virtual types (git-remote,git-submodule,fs-symlink,git-tree-entry) are introduced by #662. This PR must merge after #662." and "Recommended merge order: #662 → #661 → #663". The_resource_registry_virtual_deferred.pymodule comment also documents the #662 dependency. Clear.P1-2 (Constructor auto-bootstrap removed without migration path): RESOLVED
resource_registry_service.pyline 105 showsself.bootstrap_builtin_types()is called in__init__with the try/except fallback for pre-migration scenarios. The PR body explicitly states "Auto-bootstrap inResourceRegistryService.__init__is preserved (NOT removed)". Confirmed in the code.P3-4 (resource_type.py at 486 lines): RESOLVED
resource_type.pyis now 476 lines. Theresource_registry_service.pyis 470 lines. Both under 500. The type definitions are extracted to_resource_registry_virtual_deferred.py(115 lines).Remaining Findings
P2-3 (symlink → git-tree-entry is too broad): PERSISTS — Downgraded to P3
symlink.yamlstill haschild_types: ["fs-symlink", "git-tree-entry"]wheregit-tree-entrycovers all tree entry types, not only symlinks. The YAML description does clarify "(mode 120000)" to indicate the intent, and the runtime distinction is a filter concern, not a type-level one. This is a spec-level design decision. Downgraded from P2 to P3 (informational).New Findings
NEW P3: YAML files missing spec reference comments
The #661 YAML files have spec reference comments at the top (e.g.,
# Spec reference: docs/specification.md ~lines 10133-10143, 24548-24632), but the #663 YAML files (remote.yaml,submodule.yaml,symlink.yaml) do not. Minor inconsistency — recommend adding for parity.NEW P2: Virtual type validation is stronger than #661 — good, but creates merge-order sensitivity
#663's
_validate_model(resource_type.py lines 314-354) adds strict checks:criteriamust exist, must be a list, must be non-empty;user_addablemust be false;sandbox_strategymust benone;handlermust beNone; all capabilities must be false. This is excellent and stricter than #661's validation. However, if merge order is violated (#663 before #661), #661 would overwrite this with its weaker validator. The stated merge order (#662 → #661 → #663) handles this correctly — just noting it as a dependency.Verdict
3 of 4 original findings resolved. 1 downgraded/accepted. 2 new minor findings.
Both new findings are low-risk given the documented merge order. The strict virtual-type validation in this PR is a clear improvement. PR is in good shape.
Re-Review — PR #663
feat(resource): add deferred virtual resource typesReviewer: @brent.edwards | Round 2 — checking resolution of 4 prior findings
Prior Findings Status
Depends on: #662and recommended merge order (#662 → #661 → #663)bootstrap_builtin_types()confirmed still in__init__New Findings (2)
P3: YAML files missing
# Spec reference:comments — #661's YAMLs now have them, but #663'sremote.yaml,submodule.yaml,symlink.yamldo not. Inconsistency.P2: Validation merge-order sensitivity — #663 has stricter virtual-type validation than #661. If merge order is violated (#663 before #661), #661's looser check would overwrite #663's strict check. The documented merge order mitigates this, but it's a latent risk.
Verdict: APPROVED — All P0/P1 findings resolved. The merge dependency is well-documented. Two minor items noted for follow-up.
8a016a48f8dc72b79715New commits pushed, approval review dismissed automatically according to repository settings
dc72b797157a2182505fResponse to Review Findings (Round 3)
Rebased onto current master (now includes #661 virtual core types). All findings addressed. Pushed
7a218250.@aditya findings
F-001 (P1): Manual registration guard — ALREADY EXISTS
The
register_resource()guard at_resource_registry_ops.py:112-116already rejects types withuser_addable=False:This existed before this PR. What was missing was tests proving it works for our 3 virtual types. Added in F-003.
F-002 (P1): child_types dependency — DOCUMENTED
Already documented in PR body and commit message. Merge order enforced: #662 → #661 → #663. #661 just merged.
F-003 (P2): Negative tests for manual add rejection — ADDED
Added 3 new scenarios:
Each bootstraps an in-memory DB, then attempts
register_resource(type_name=...)and asserts the error contains "not user-addable".F-004 (P2): Tighten criteria validation — DONE
Added per-element validation in
resource_type.py:408-416: each entry incriteriamust be astrand must be non-empty after stripping whitespace. Added 2 new negative test scenarios:[123])[""])F-005 (P3): PR metadata mismatches — FIXED
Updated PR description to reflect: 52 scenarios (not 42), 7 Robot tests, extracted
_resource_registry_virtual_deferred.py, runtime guard documentation.@brent.edwards findings (Round 2)
YAML spec references missing — ADDED
All 3 YAML files now have
# Spec reference: docs/specification.md ~lines 23705-23719, 24619-24631at the top, matching #661's pattern.Validation merge-order sensitivity — MITIGATED
#661 just merged with its own (weaker) validation. Our stricter validation (at
resource_type.py:390-443) will supersede it when #663 merges. The stated merge order ensures this is safe.Current state: 52 scenarios, 200 steps, lint clean, typecheck 0 errors, PR mergeable at
7a218250.7a2182505f5d6cb099ad