feat(resource): add virtual core resource types #661
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
5 participants
Notifications
Due date
No due date set.
Blocks
#329 feat(resource): add virtual core resource types
cleveragents/cleveragents-core
Reference
cleveragents/cleveragents-core!661
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature/post-resource-types-virtual-core"
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 #329 -- adds 6 built-in virtual core resource types (
file,directory,commit,branch,tag,tree) to the resource registry. These virtual types represent abstract identity nodes that bridge multiple physical resource representations through equivalence metadata.Motivation
Physical resource types like
fs-fileandgit-tree-entryrepresent the same logical concept (a file) in different contexts. Virtual core types provide a stable identity layer: a virtualfilecan be linked to bothfs-fileandgit-tree-entryphysical resources via equivalence tracking (#334). This enables cross-context resource resolution -- e.g., finding all physical representations of the same logical file across git history and filesystem.Changes
YAML Configs (
examples/resource-types/)file.yaml-- equivalence by content_hash + filename + permissionsdirectory.yaml-- equivalence by merkle_hash + name + permissionscommit.yaml-- equivalence by commit_hash (git SHA)branch.yaml-- equivalence by name + head_commit_hashtag.yaml-- equivalence by name + target_shatree.yaml-- equivalence by tree_hashAll set
resource_kind: virtual,user_addable: false,sandbox_strategy: none,built_in: true.Bootstrap Registration (
resource_registry_service.py)_BUILTIN_TYPESunder# === Virtual Core Resource Types (#329) ===section marker (for merge coordination with #330, #331)BUILTIN_NAMESfrozenset updated inresource_type.pyDocumentation (
docs/reference/resource_types_builtin.md)Tests
features/resource_type_virtual_core.feature-- 83 scenarios covering YAML validation, type properties, equivalence metadata, bootstrap registration, BUILTIN_NAMESrobot/resource_type_virtual_core.robot-- 4 integration testsbenchmarks/resource_type_virtual_core_bench.py-- schema load, domain model, cli_dict benchmarksKey Decisions
filenotv-file) per spec lines 10133-10141Automated Check Results
Diff Coverage
resource_registry_service.py(delta)resource_type.py(delta)Spec Reference
docs/specification.mdlines 10133-10141, 23540-23554, 24459Closes #329
Merge Coordination
Merge order: This PR can merge independently — it does not require #662 first.
Forward references in
child_types: Virtual types reference physical types from #662 (e.g.git-tree-entry,git-branch). This is safe becausebootstrap_builtin_types()andspec_to_db()storechild_typesas opaque JSON strings with no referential-integrity validation. The forward references become concrete after #662 merges. See comment in_resource_registry_virtual.py.BUILTIN_NAMESconflict mitigation: The docstring inresource_type.pyhas been trimmed from 496 to 448 lines (48 lines freed) to give headroom for #662 (+28 lines) and #663 (+53 lines). After all three merge, the file will be ~529 lines — may need one more extraction.BUILTIN_NAMESentries are alphabetically sorted to minimize diff conflicts.Registration order: Virtual types are spliced after
DATABASE_TYPE_DEFSinBUILTIN_TYPES. Registration order does not matter —bootstrap_builtin_types()does not validate parent/child existence at registration time.bd4e506f167e2e296253PM 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: Virtual core resource types (file, directory, commit, branch, tag, tree). M7 (v3.6.0), due Mar 28. Author: @hamza.khyari. Thorough PR body with 83 BDD scenarios, 100% diff coverage.
Action Required: @hamza.khyari — Rebase onto current master. These 4 PRs (#661-#664) share a common merge-base and likely need to be rebased and merged sequentially. Prioritize #657's P1 fixes first, then this series. Labels incomplete — missing MoSCoW, Points.
Rebase Required
@hamza.khyari — This PR has merge conflicts with
masterand cannot be merged in its current state. Please rebase onto the latestmasterand force-push to resolve the conflicts.This is one of 4 PRs from you that currently have conflicts (#661, #662, #663, #664). Please prioritize rebasing the ones closest to merge-ready.
PM Status Update — Day 34
This is the foundation of the resource type chain (#661 → #662 → #663 → #664). All 4 are blocked on merge conflicts.
@hamza.khyari — Please rebase this PR onto current master and force-push. This should be rebased first in the series since #662-#664 depend on it. After this merges, cascade the rebases through the chain.
No code review findings exist — the quality looks solid (83 BDD scenarios, 100% diff coverage). Once rebased, this should be quick to review and merge.
Priority: Medium (M7, due Mar 28 — on track if rebased this week)
PM Notice — Day 34 (2026-03-14): Merge conflict detected
@hamza.khyari This PR has merge conflicts with master. Please rebase onto current master when you have bandwidth. This is M7 work (MoSCoW: Could have) so it is not on the critical path — no urgency, but please rebase before the M7 due date (Mar 28).
Note: PRs #662, #663, #664 have the same conflict. If these share a common base, rebasing #661 first and then the others onto it may be the cleanest approach.
PM Day 36: Virtual core resource types. Closes #329. M7 scope. First in resource type chain (#661->#662->#663->#664). @hamza.khyari author. Reviewer: @freemo.
7e2e296253a90299767da90299767d6bf4c115ed6bf4c115ed8a1a9f3bbe8a1a9f3bbe372780e412PR #661 Review — Virtual Core Resource Types
Summary
Adds 6 virtual core types (file, directory, commit, branch, tag, tree). Code is clean, well-documented, tests are thorough (83 scenarios). However, there are merge-coordination issues that must be resolved before this can land.
P0 — Must Fix
1. Guaranteed three-way merge conflict on
BUILTIN_NAMESfrozensetresource_type.py: This PR adds 6 names and a# Physical built-in typessection comment. PR #662 rewrites the entire frozenset with alphabetical sorting and different section comments. PR #663 appends 3 more names at the end. Whichever merges second/third will hit a textual conflict across the entireBUILTIN_NAMESblock.Action required: Coordinate merge order with #662 and #663. Recommend merging #662 first (it does the rewrite), then rebasing #661 and #663 onto it.
2. Guaranteed merge conflict on
_resource_registry_data.pylist tailThis PR splices
*VIRTUAL_CORE_TYPESafter*DATABASE_TYPE_DEFS. PR #662 splices*DEFERRED_PHYSICAL_TYPESbefore*DATABASE_TYPE_DEFS. PR #663 inlines types after*DATABASE_TYPE_DEFS. All three touch the same 3-line region at the end of theBUILTIN_TYPESlist, plus add different imports at the top. Same conflict story on the docstring table inresource_registry_service.py.3. Dangling
child_typesreferencesVirtual
filedeclareschild_types: ["fs-file", "git-tree-entry"]. Virtualdirectoryandtreedeclarechild_typesreferencing"git-tree". Neithergit-tree-entrynorgit-treeexist on master — they are introduced by PR #662. If #661 merges before #662, these are dangling references. Does the registry validate child_type existence at bootstrap time? If so, this will fail at runtime.Action required: Either (a) merge #662 first, or (b) confirm that the registry is lenient about forward-referencing child types, and document this explicitly.
P1 — Should Fix
4.
resource_type.pyat 496 lines — 4 lines from the 500-line CONTRIBUTING limitThe 53-line docstring expansion is the main contributor. If #662 or #663 also adds to this file (they do — #662 adds 28 lines, #663 adds 53 lines), the post-merge file will exceed 500 lines. This is a structural issue across the PR series.
Suggestion: The expanded docstring (Built-in vs Custom Types, Validation Rules sections) is useful but could live in a separate
docs/reference file rather than inline in the module docstring. This would free ~40 lines.P2 — Nice to Have
5.
list[dict[str, Any]]typing forVIRTUAL_CORE_TYPESConsistent with the existing
BUILTIN_TYPESpattern, but this is the weakest possible typing. ATypedDictfor the resource type dict shape would catch key typos at type-check time. Not a blocker for this PR since it matches existing code, but worth a follow-up issue.6. Splice order: virtual types after DATABASE_TYPE_DEFS
Virtual types that reference physical
child_typesare registered after their children (physical types come first in the list). Confirm that registration order doesn't matter for the bootstrap — if it does, virtual types should come last.P3 — Observations
resource_kind: virtual,sandbox_strategy: none,user_addable: false,built_in: true, all capabilities false. Equivalence criteria are well-chosen.feat(resource): add virtual core resource types._resource_registry_virtual.py198 lines,_resource_registry_data.py399 lines — well under limits.filenotv-file) per spec lines 10133-10141 — correct decision.Verdict: Request Changes
P0 items (merge conflicts and dangling child_types) must be addressed. Recommend establishing a merge order across the three PRs (#662 → #661 → #663) and rebasing accordingly.
@ -196,6 +199,8 @@ BUILTIN_TYPES: list[dict[str, Any]] = [},},*DATABASE_TYPE_DEFS,# Virtual core resource types -- see _resource_registry_virtual.py (#329)P1:
*VIRTUAL_CORE_TYPESis spliced after*DATABASE_TYPE_DEFS. PR #662 adds*DEFERRED_PHYSICAL_TYPESbefore*DATABASE_TYPE_DEFS. Confirm registration order doesn't matter, or align the splice points across the three PRs.@ -0,0 +45,4 @@"sandbox": False,"checkpoint": False,},"equivalence": {P0:
child_typeshere referencegit-tree-entryandgit-tree(lines ~48, ~70, ~163, ~192) which don't exist on master. They are introduced by PR #662. If this PR merges first, these are forward references. Confirm that the registry tolerates dangling child_type names at bootstrap, or establish #662 as a merge prerequisite.@ -155,6 +208,7 @@ class ResourceTypeSpec(BaseModel):# Built-in resource type names (unnamespaced)BUILTIN_NAMES: ClassVar[frozenset[str]] = frozenset({P0: This
BUILTIN_NAMESfrozenset is modified by all three PRs (#661, #662, #663). PR #662 rewrites the entire block with alphabetical sorting. This will be a guaranteed merge conflict. Coordinate merge order — suggest #662 first since it does the structural rewrite.Code Review — PR #661
feat(resource): add virtual core resource typesReviewer: @brent.edwards | Size: L (210 new lines) | Focus: Registry, YAML configs, merge coordination
P1:must-fix (2)
1.
BUILTIN_NAMESfrozenset will triple-conflict with #662 and #663All three PRs (#661, #662, #663) independently rewrite the
BUILTIN_NAMESfrozenset inresource_type.pyand add entries to_BUILTIN_TYPESin the registry. They MUST be merged in a strict order (#662 → #661 → #663) with a rebase after each merge. This dependency is not documented in any PR description. If merged out of order, the conflictingBUILTIN_NAMESwill silently lose types from whichever PR merged first.Fix: Document the required merge order in all 3 PR descriptions. Consider extracting
BUILTIN_NAMESto a separate_builtin_names.pymodule that each PR appends to via set union.2.
child_typesreference types that only exist after #662 mergesVirtual core type
filedeclareschild_types: ["fs-file", "git-tree-entry"].git-tree-entryis introduced by PR #662 (deferred physical types). If #661 merges first, bootstrap registration will either fail or create a type referencing a nonexistent child. Similarly,tree→git-tree,commit→git-commit,branch→git-branch,tag→git-tagall depend on #662.Fix: #662 must merge before #661. Add a
Depends on: #662note to PR description.P2:should-fix (2)
3.
equivalence_metadatausesdict[str, Any]— theidentity_keyandidentity_fieldspatterns are identical across all 6 types but have no shared validation. A typo inidentity_key(e.g.,"content_hah") would silently pass. Consider aEquivalenceMetadataSchemaPydantic model.4.
resource_type.pyat 499 lines — one line from the limit. The next PR (#663) adds 3 more names and will push it over. Plan for extraction now.P3:nit (1)
5. YAML files lack a
# Spec reference:comment linking to the specification line numbers cited in the PR description.Positive Observations
filenotv-file) are correct per spec S10133-10141Verdict: REQUEST_CHANGES — merge order dependency (P1-1, P1-2) must be documented before merge.
372780e412ce66c75275Response to Review Findings
Thanks @brent.edwards for the thorough review. All items addressed in
ce66c752:P0-1 / P1-1: Merge order and BUILTIN_NAMES conflicts
Investigated:
bootstrap_builtin_types()storeschild_typesas opaque JSON strings viaspec_to_db(). There is zero referential-integrity validation ofchild_typesorparent_typesat any layer (ResourceTypeSpec,from_config(),_validate_model,bootstrap_builtin_types(),register_type()). Forward references are safe.This PR can merge before #662. Added a "Merge Coordination" section to the PR description documenting this, and a code comment in
_resource_registry_virtual.pyexplaining the forward-reference safety.BUILTIN_NAMESentries are now alphabetically sorted to minimize diff conflicts across the series.P0-2 / P1-2: Dangling child_types
Confirmed safe — see above. Added explicit documentation comment in
_resource_registry_virtual.py(lines 27-33) citingbootstrap_builtin_types()andspec_to_db()as evidence.P1-4 / P2-4: resource_type.py at 496 lines
Fixed. Trimmed the module docstring from ~53 lines of inline documentation to a concise 16-line summary. The detailed built-in type catalogue, validation rules, and examples now live in
docs/reference/resource_types_builtin.md(which this PR already maintains). File went from 496 → 448 lines — 48 lines freed for #662 (+28) and #663.P2-3 / P2-5: TypedDict for equivalence / BUILTIN_TYPES
Agreed this would be better. The
dict[str, Any]pattern is inherited from the existingBUILTIN_TYPESdesign. Filed as a follow-up concern — not blocking for this PR since it matches the existing code pattern.P1-6: Splice order (virtual after DATABASE_TYPE_DEFS)
Confirmed safe.
bootstrap_builtin_types()iteratesBUILTIN_TYPESsequentially and callsResourceTypeSpec.from_config()+spec_to_db()for each. It never validates that parent/child types are already registered — onlyinheritschains are validated (viavalidate_chain()). Registration order is irrelevant.P3-5: YAML spec reference comments
Fixed. Added
# Spec reference: docs/specification.md ~lines 10133-10141, 23540-23554to all 6 YAML files.PR Review: !661 (Ticket #329)
Verdict: Request Changes
The PR is well-implemented overall — the 6 virtual core resource types are correctly structured, tests are thorough (85 Behave scenarios, 4 Robot tests, ASV benchmarks), code is clean, and all ticket acceptance criteria are met functionally. However, there are 4 major issues (including documentation/traceability concerns and unchecked Definition-of-Done subtasks) and several minor gaps that should be addressed before merge.
Critical Issues
None
Major Issues
1. Incorrect spec line references in all 6 YAML files and PR description
examples/resource-types/{file,directory,commit,branch,tag,tree}.yaml, line 1# Spec reference: docs/specification.md ~lines 10133-10141, 23540-23554. While lines 10133–10141 correctly reference the built-in type table, lines 23540–23554 reference the A2A authentication/versioning section — completely unrelated to virtual resource types. The actual virtual type equivalence semantics are at ~lines 24548–24632 (equivalence linking section and identity key table). The PR description's "Spec Reference" section also cites the wrong range.~lines 10133-10143, 24548-24632.2. Documentation omits
merkle_hashcriterion fordirectoryequivalence tabledocs/reference/resource_types_builtin.md, lines ~227–233 (directory equivalence table)directorylists onlynameandpermissions, omittingmerkle_hash— which is the primary identity criterion. Both the YAML (directory.yamlline 19) and the Python registration (_resource_registry_virtual.pyline 88) includemerkle_hashas the first criterion. The prose text mentions Merkle hash but the formal table is incomplete.merkle_hashrow to the directory equivalence table:3. Wrong spec section reference in
resource_types_builtin.mddocs/reference/resource_types_builtin.md, line 18See docs/specification.md section "Physical vs Virtual Resources" (~line 24385)but line 24385 is in the middle of a PlantUML diagram object definition (object "git-branch" as grBranch). The actual "Physical vs Virtual Resources" section starts at ~line 24548.(~line 24385)to(~line 24548).4. Ticket Definition of Done not satisfied — two subtasks remain unchecked
File: Ticket #329 subtask checklist
Problem: Two subtasks are unchecked:
[ ] Verify coverage >=97% via nox -s coverage_report[ ] Run nox (all default sessions)The ticket's Definition of Done explicitly states: "All subtasks below are completed and checked off." CONTRIBUTING.md "Commit Scope and Quality" (line 113) says: "Only commit when a piece of functionality is fully implemented and tested." CONTRIBUTING.md "Quality Gates" (line 67): "Do not consider work complete until all tests pass, all documentation is updated, and all associated quality checks succeed."
The PR body claims all checks pass with 100% diff coverage, but the ticket checklist contradicts this by leaving verification subtasks unchecked.
Recommendation: Run
nox(all default sessions) andnox -s coverage_report, verify they pass, and check off the subtasks before merge.Minor Issues
5.
directoryequivalence criteria deviate from spec identity tableexamples/resource-types/directory.yamllines 19–21;_resource_registry_virtual.pylines 83–93"Merkle hash of recursive child identities". The implementation addsnameandpermissionsas additional criteria, making it stricter than the spec. The code comment is good but the YAML description (line 22) only mentions "same Merkle hash" without mentioning name/permissions.6. Missing
merkle_hashassertion in directory equivalence Behave testfeatures/resource_type_virtual_core.feature, lines 189–194nameandpermissionsbut does not assertmerkle_hash. A regression removingmerkle_hashwould go undetected.And the vcore equivalence criteria should contain "merkle_hash".7. Bootstrap registration property assertions cover only 3 of 6 types
features/resource_type_virtual_core.feature, lines 251–264file,commit, andtagrespectively. A bug in another type's registration would not be caught.8. No equivalence criteria count assertions in any test
features/resource_type_virtual_core.feature, lines 181–2209. Negative test catches bare
Exceptioninstead of specific error typefeatures/steps/resource_type_virtual_core_steps.py, lines 329–334step_attempt_create_speccatchesExceptionbroadly, which could mask unexpected non-validation errors.(ValueError, pydantic.ValidationError).10. Imports inside
setup()methods in benchmarksbenchmarks/resource_type_virtual_core_bench.py, lines 20–21, 40–42, 65–6711. Inline import inside Behave step function
features/steps/resource_type_virtual_core_steps.py, line 44from ... import BUILTIN_TYPESinside function body violates CONTRIBUTING.md import guidelines.12. No test guards against YAML ↔ registry data drift
features/resource_type_virtual_core.feature(general gap)_resource_registry_virtual.pyare manually synchronized duplicate data. YAML tests validate YAMLs; bootstrap tests validate Python dicts. No test asserts they are equivalent. If someone updates one but not the other, all tests pass while data silently diverges.VIRTUAL_CORE_TYPESentry, asserting key fields match.13. Domain model scenarios don't verify equivalence preservation
features/resource_type_virtual_core.feature, lines 224–238nameandresource_kindbut never checks thatequivalencesurvives thefrom_config()round-trip. Iffrom_config()silently dropped equivalence, these tests would still pass.And the vcore domain model should have equivalenceto the domain model Scenario Outline.14. "Empty criteria" negative test doesn't validate the created spec
features/resource_type_virtual_core.feature, lines 316–319Nonewould pass.context.vcore_created_spec.equivalence["criteria"] == [].15. CLI output helpers omit
equivalencefield — virtual type details invisible to userssrc/cleveragents/cli/commands/resource.py, lines 115–142 (_resource_type_dict) and 393–437 (_print_type_panel)ResourceTypeSpecbut skipequivalence. Runningagents resource type show filewon't display equivalence criteria — the defining characteristic of virtual types.ResourceTypeSpec.as_cli_dict()correctly includes it, but the CLI doesn't useas_cli_dict().spec.as_cli_dict()or add equivalence to the helpers. Could be a follow-up issue rather than blocking this PR, since it's pre-existing code.Nits
16. Unused
_VIRTUAL_CORE_NAMESconstantfeatures/steps/resource_type_virtual_core_steps.py, lines 25–2717.
VIRTUAL_CORE_TYPES: list[dict[str, Any]]uses weakest possible typingsrc/cleveragents/application/services/_resource_registry_virtual.py, line 34TypedDict.18.
directory.yamlequivalence description inconsistent with criteriaexamples/resource-types/directory.yaml, line 2219. Missing
encoding="utf-8"in benchmarkopen()callsbenchmarks/resource_type_virtual_core_bench.py, lines 47 and 7120. Docstring table alignment in
resource_registry_service.pysrc/cleveragents/application/services/resource_registry_service.py, line 1821. YAML example files omit
handlerfieldexamples/resource-types/handleras required. The Python dicts correctly include"handler": None, but the YAML examples omit it. Users referencing these as templates would create configs missing a required field.handler: nullto each YAML file for completeness.22. Benchmark classes missing
timeoutattributebenchmarks/resource_type_virtual_core_bench.py, lines 16, 36, 61timeout = 60. This file omits it.timeout = 60to each benchmark class.23. PR description claims 83 scenarios but actual count is 85
24. Inconsistent type annotation
dict[str, object]vsdict[str, Any]features/steps/resource_type_virtual_core_steps.py, line 65dict[str, object]while every other dict annotation in the file usesdict[str, Any].dict[str, Any]for consistency.25. Shallow copy of
equivalenceinas_cli_dict()src/cleveragents/domain/models/core/resource_type.py, line 438dict(self.equivalence)shares the innercriterialist. Callers mutating the returned dict would silently modify domain model state.copy.deepcopy(self.equivalence)or document as read-only. Pre-existing, no current callers mutate.Observations (Not Issues)
# type: ignore[attr-defined]comments in step definitions: Established codebase-wide pattern for Behave's untypedcontext. Not introduced by this PR.register_resource()(doesn't enforceuser_addable: false) andregister_type()(doesn't prevent external YAML claimingbuilt_in: true). Surface area increases but not introduced here.bootstrap_builtin_types(): Pre-existing O(n) redundant SELECTs; now 6 more with virtual types. Negligible on SQLite.resource_kindenum vs spec'sphysical: bool: Pre-existing structural divergence between spec schema and implementation. New YAMLs follow the implementation pattern, not the spec schema.filenotv-file) are correct per spec lines 10133–10143.Summary
This is a well-crafted PR with thorough testing and clean code. The 6 virtual core resource types are correctly structured, properly registered, and comprehensively tested. The 4 major issues are: (1-3) three separate incorrect spec line references across YAML files and documentation, and (4) two unchecked Definition-of-Done subtasks per CONTRIBUTING.md requirements. The 11 minor issues include test coverage gaps (missing merkle_hash assertion, no YAML↔registry drift guard, incomplete bootstrap property checks, no equivalence count/preservation assertions), import guideline violations, and a CLI display gap for equivalence data. The 10 nits are cosmetic or pre-existing concerns. Once the major issues and the more impactful minor items are addressed, this PR should be ready to merge.
Code Review — PR #661
feat(resource): add virtual core resource typesREQUEST_CHANGESThis PR delivers the core virtual type configs and registration wiring, but it still has multiple merge-blocking quality gaps versus issue #329 and project policy: explicit type-check suppression comments in new test code, acceptance criteria not actually proven by integration-level tests, and a documentation/spec mismatch in virtual directory equivalence criteria. There is also an unresolved PR dependency risk around virtual child types referencing physical types not present on current
master. The branch is close, but these must be addressed before merge.Findings Table
# type: ignore[...]), which violates repository type-safety policy.features/steps/resource_type_virtual_core_steps.pycontains many# type: ignore[attr-defined];CONTRIBUTING.mdforbids inline type suppression.type: ignore.robot/resource_type_virtual_core.robotimportsBUILTIN_TYPESdirectly in embedded Python.agents resource type list --format json) and assert the six virtual types in output data.user_addable: falsemetadata.features/resource_type_virtual_core.featurehas metadata assertions only; no scenario attemptsagents resource add file(or service equivalent) and validates rejection path.directorydocs omitmerkle_hashin equivalence criteria table, conflicting with YAML/registry config and issue acceptance criteria for directory equivalence metadata.docs/reference/resource_types_builtin.mddirectory criteria table listsnameandpermissions, whileexamples/resource-types/directory.yamland_resource_registry_virtual.pyincludemerkle_hash.merkle_hash(and keep semantics consistent with config + spec wording).git-tree,git-tree-entry,git-commit,git-branch,git-tag) that are not present in currentmasterbuilt-in set; dependency/order is not declared in PR metadata._resource_registry_virtual.pychild types reference git physical types; currentmasterResourceTypeSpec.BUILTIN_NAMESlacks those names; PR text does not declare hard dependency chain.features/resource_type_virtual_core.featureincludes scenario "Virtual type with empty criteria list is accepted for vcore".ResourceTypeSpecvalidation to reject emptyequivalence.criteriafor virtual types, and update scenario to expect failure.Consolidated Checklist
P0
type: ignoresuppressions fromfeatures/steps/resource_type_virtual_core_steps.pyand pass type checks policy-compliantly.P1
resource type listbehavior through CLI output, not direct module internals.directoryequivalence documentation to includemerkle_hashand align with YAML/registry.P2
equivalence.criteriafor virtual types and update tests accordingly.P3
Fix Plan
features/steps/resource_type_virtual_core_steps.pyto remove alltype: ignoreusage (typed context wrapper/protocol).resource type listand assert six virtual names in CLI output.docs/reference/resource_types_builtin.mddirectory equivalence table to includemerkle_hash.equivalence.criteriafor virtual types and update failing/success expectations in feature scenarios.Dependency notes:
ce66c752753eb432003d3eb432003d681c927814Combined Response to @hurui200320 and @aditya Reviews
All findings from both reviews addressed in
4e0091ba. Verified line-by-line against the pushed code. All tests pass (95 + 115 scenarios, 0 failures).@hurui200320 — 25 Items
Major (4)
~lines 10133-10143, 24548-24632merkle_hashin directory docs tabledocs/reference/resource_types_builtin.mdnow hasmerkle_hashrow(~line 24548)noxsuite execution.nox -s lintandnox -s typecheckpass. The two remaining subtasks (noxall sessions,coverage_report) require the full test infrastructure and will be checked off after execution.Minor (11)
merkle_hashassertionExceptioncatch(ValueError, ValidationError)setup()BUILTIN_TYPESimport at topshould have equivalencestep_resource_type_dict()and_print_type_panel()Nits (10)
_VIRTUAL_CORE_NAMESlist[dict[str, Any]])BUILTIN_TYPESpattern. Same typing used across entire registry.encoding="utf-8"open()callsgit-checkout/fs-directoryalignmenthandlerfieldhandler: nulladded to all 6timeouttimeout = 60on all 3 classesdict[str, object]inconsistencydict[str, Any]as_cli_dict()deep-copies via list comprehension@aditya — 6 Items
type: ignoresuppressions_Ctxtyped proxy class. 0 runtime# type: ignoresuppressions (was 51). Satisfies both Pyright and ruff B009/B010.Virtual Types Appear In CLI Resource Type List(bootstraps service, callslist_types(), verifies all 6 types with correct kind/user_addable/equivalence — same code path asresource type list --format json) andVirtual Types Are Rejected By Register Resource(proves rejection viaregister_resource()).user_addableenforcement guard inregister_resource()(_resource_registry_ops.py:111-115). RaisesValidationError("not user-addable"). Added Behave scenario (Attempting to register a virtual resource instance is rejected) and Robot test (Virtual Types Are Rejected By Register Resource) both proving virtual types are rejected at the service layer.merkle_hash_validate_model()now rejects emptyequivalence.criteriafor virtual types (only whencriteriakey is present). Test updated to expect rejection. Existing tests unaffected (verified: 115 consolidated_resource scenarios pass).681c9278142c45812c732c45812c734e0091ba5cPM Status — Day 37
Status: BLOCKED — merge conflicts + active review cycle. @hamza.khyari pushed fixes (
ce66c752,4e0091ba) addressing all findings from @hurui200320 (25 items) and @aditya (6 items including P0 type-ignore policy blocker).Key progress (Day 37):
type: ignoresuppressions removed via typed_Ctxproxyequivalence.criterianow rejected for virtual typesBlockers:
Action items:
Priority: Medium (M7 v3.6.0, due Mar 28 — on track if rebased this week).
PM status comment — Day 37
Re-Review (Round 2) — PR #661
Checked the force-pushed update (
4e0091ba) against the 5 findings from Round 1.Resolved Findings
P1-1 (BUILTIN_NAMES triple-conflict with #662/#663): RESOLVED
The PR body now has a thorough "Merge Coordination" section documenting: merge independence, alphabetical sorting of
BUILTIN_NAMESto minimize diff conflicts, and docstring trimmed from 496→448 lines to give headroom for #662/663. Well done.P1-2 (child_types reference types from #662): RESOLVED
_resource_registry_virtual.pylines 27-31 has an explicitNOTEcomment explaining thatchild_typesentries likegit-tree-entry,git-branch, etc. are forward references to #662, stored as opaque strings with no referential-integrity check. Also documented in the PR body. Clear and sufficient.P2-4 (resource_type.py at 499 lines): RESOLVED
resource_type.pyis now 457 lines (down from 499). Type definitions extracted to_resource_registry_virtual.py(205 lines), data helpers to_resource_registry_data.py(399 lines), ops to_resource_registry_ops.py(246 lines). Good decomposition.P3-5 (YAML files lack spec reference): RESOLVED
All 6 YAML files now have spec reference comments at the top (e.g.,
# Spec reference: docs/specification.md ~lines 10133-10143, 24548-24632).Remaining / New Findings
P2-3 (equivalence uses
dict[str, Any]): PERSISTS — Accepted as-isequivalence: dict[str, Any] | Noneis still untyped. A Pydantic sub-model would be better for self-documentation, but runtime validation is present and #663 strengthens it further. No action required for this PR.NEW P2: Virtual type validation allows missing
criteriakeyresource_type.pyline 327:This only catches empty criteria. A virtual type with
equivalence: {"description": "foo"}(nocriteriakey) passes validation silently. Compare with #663 which correctly doesif "criteria" not in self.equivalence. Since #663 merges last per the stated order (#662 → #661 → #663), this gap will be closed — but during the window between #661 and #663 merging, the validation is incomplete. Recommend aligning with #663's stricter check:Verdict
4 of 5 original findings resolved. 1 accepted. 1 new P2 found (validation gap).
The P2 validation gap is low-risk given the stated merge order, but fixing it now avoids a temporary regression window. Otherwise, this PR is in good shape.
Re-Review — PR #661
feat(resource): add virtual core resource typesReviewer: @brent.edwards | Round 2 — checking resolution of 5 prior findings
Prior Findings Status
_resource_registry_virtual.py+ PR body explains forward-ref safety (opaque JSON, no referential integrity validation at bootstrap)dict[str, Any]# Spec reference:commentsNew Finding (1)
P2:
criteriavalidation gap — The virtual-type checkif "criteria" in ... and not ...misses the case where thecriteriakey is absent entirely. #663's stricter validation covers this, but in the interim state after #661 merges, a virtual type withoutcriteriapasses validation. Consider adopting #663's check proactively.Verdict: APPROVED — All P0/P1 findings resolved. The merge coordination documentation is thorough and the forward-reference safety is well-explained. One minor P2 noted.
4e0091ba5c4f0d1b218aNew commits pushed, approval review dismissed automatically according to repository settings
4f0d1b218a241c428994241c428994c14ce65d61