cleveragents/cleveragents-core
4
0
Fork 3
Code Issues 6.0k Pull Requests 115 Actions Packages Projects Releases Wiki Activity

feat(resource): add deferred virtual resource types #663

Merged
hamza.khyari merged 1 commits from feature/post-resource-types-virtual into master 2026-03-18 02:40:32 +00:00
Conversation 13 Commits 1 Files Changed 16
+1534 -16
hamza.khyari commented 2026-03-10 00:40:46 +00:00
Member
Copy Link
Copy Source

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_types referenced 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)

  • 3 virtual types extracted to a dedicated module for consistency with #661/#662
  • remote -- equivalence criteria: ["url"]; children: git-remote
  • submodule -- equivalence criteria: ["url", "path"]; children: git-submodule
  • symlink -- equivalence criteria: ["name", "target_path"]; children: fs-symlink, git-tree-entry
  • All: resource_kind: virtual, user_addable: false, sandbox_strategy: none, built_in: true, handler: None, all capabilities false

YAML Configs (examples/resource-types/)

  • remote.yaml, submodule.yaml, symlink.yaml with spec reference comments

Model Validation (resource_type.py)

  • Enhanced virtual type validation:
    • criteria must be a non-empty list[str] with each element a non-empty string
    • user_addable must be false, sandbox_strategy must be none
    • handler must be None, all capabilities must be false

Runtime Guard

  • Existing register_resource() guard at _resource_registry_ops.py:112-116 rejects manual creation of types with user_addable=False
  • 3 new test scenarios prove this guard works for remote, submodule, symlink

Tests

  • Behave: 52 scenarios -- type properties, equivalence metadata, domain model loading, DB roundtrip via bootstrap, manual add rejection (3 scenarios), negative validation (missing equivalence/name/kind, non-string criteria, empty-string criteria)
  • Robot: 7 integration tests (YAML validation, domain models, DB bootstrap roundtrip, negative validation)
  • ASV: 3 benchmark classes, 10 methods

Documentation

  • docs/reference/resource_types_builtin.md -- deferred virtual types section with equivalence metadata tables

Equivalence Metadata (Critical for #334)

Type criteria description
remote ["url"] Normalized URL identity
submodule ["url", "path"] Submodule URL + path identity
symlink ["name", "target_path"] Symlink name + target path identity

Spec Reference

docs/specification.md ~line 23705 (virtual types table), lines 24619-24631 (identity key table)

Closes #331

## 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_types` referenced 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`) - 3 virtual types extracted to a dedicated module for consistency with #661/#662 - `remote` -- equivalence criteria: `["url"]`; children: `git-remote` - `submodule` -- equivalence criteria: `["url", "path"]`; children: `git-submodule` - `symlink` -- equivalence criteria: `["name", "target_path"]`; children: `fs-symlink`, `git-tree-entry` - All: `resource_kind: virtual`, `user_addable: false`, `sandbox_strategy: none`, `built_in: true`, `handler: None`, all capabilities false ### YAML Configs (`examples/resource-types/`) - `remote.yaml`, `submodule.yaml`, `symlink.yaml` with spec reference comments ### Model Validation (`resource_type.py`) - Enhanced virtual type validation: - `criteria` must be a non-empty `list[str]` with each element a non-empty string - `user_addable` must be `false`, `sandbox_strategy` must be `none` - `handler` must be `None`, all capabilities must be `false` ### Runtime Guard - Existing `register_resource()` guard at `_resource_registry_ops.py:112-116` rejects manual creation of types with `user_addable=False` - 3 new test scenarios prove this guard works for `remote`, `submodule`, `symlink` ### Tests - **Behave**: 52 scenarios -- type properties, equivalence metadata, domain model loading, DB roundtrip via bootstrap, manual add rejection (3 scenarios), negative validation (missing equivalence/name/kind, non-string criteria, empty-string criteria) - **Robot**: 7 integration tests (YAML validation, domain models, DB bootstrap roundtrip, negative validation) - **ASV**: 3 benchmark classes, 10 methods ### Documentation - `docs/reference/resource_types_builtin.md` -- deferred virtual types section with equivalence metadata tables ## Equivalence Metadata (Critical for #334) | Type | criteria | description | |------|----------|-------------| | `remote` | `["url"]` | Normalized URL identity | | `submodule` | `["url", "path"]` | Submodule URL + path identity | | `symlink` | `["name", "target_path"]` | Symlink name + target path identity | ## Spec Reference `docs/specification.md` ~line 23705 (virtual types table), lines 24619-24631 (identity key table) Closes #331
hamza.khyari self-assigned this 2026-03-10 00:45:37 +00:00
hamza.khyari added this to the v3.6.0 milestone 2026-03-10 00:46:51 +00:00
hamza.khyari added the
Type
Feature
Priority
Low
 labels 2026-03-10 00:47:16 +00:00
hamza.khyari force-pushed feature/post-resource-types-virtual from 8462326710 to cc1b016bef 2026-03-10 01:13:23 +00:00 Compare
hamza.khyari removed the
Priority
Low
 label 2026-03-10 01:21:58 +00:00
hamza.khyari force-pushed feature/post-resource-types-virtual from cc1b016bef to 3e4c2e8286 2026-03-10 02:29:24 +00:00 Compare
hamza.khyari added a new dependency 2026-03-10 02:45:57 +00:00
#331 feat(resource): add deferred virtual resource types
freemo commented 2026-03-11 05:50:12 +00:00
Owner
Copy Link
Copy Source

PM 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 develop and push.

Priority: M6 work — continue at pace. Your Days 30-31 velocity was excellent (5 merges).

**PM 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 `develop` and push. **Priority**: M6 work — continue at pace. Your Days 30-31 velocity was excellent (5 merges).
freemo added the
Priority
Medium
State
Unverified
 labels 2026-03-11 18:15:25 +00:00
freemo commented 2026-03-11 18:17:49 +00:00
Owner
Copy Link
Copy Source

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 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).
freemo commented 2026-03-12 20:46:35 +00:00
Owner
Copy Link
Copy Source

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.

## 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.
freemo added the
MoSCoW
Could have
Points
8
 labels 2026-03-12 20:57:11 +00:00
freemo commented 2026-03-13 22:06:17 +00:00
Owner
Copy Link
Copy Source

Rebase Required

@hamza.khyari — This PR has merge conflicts with master. Please rebase onto the latest master and force-push. See also: #661, #662, #664 (all need rebase).

## Rebase Required @hamza.khyari — This PR has merge conflicts with `master`. Please rebase onto the latest `master` and force-push. See also: #661, #662, #664 (all need rebase).
hamza.khyari added
State
In Review
 and removed
State
Unverified
 labels 2026-03-16 12:25:09 +00:00
freemo reviewed 2026-03-16 16:26:52 +00:00
freemo left a comment
Owner
Copy Link
Copy Source

PM Day 36: Deferred virtual resource types. Closes #331. M7 scope. Sequential dependency on #662. @hamza.khyari author.

PM Day 36: Deferred virtual resource types. Closes #331. M7 scope. Sequential dependency on #662. @hamza.khyari author.
hamza.khyari removed the
MoSCoW
Could have
Points
8
Priority
Medium
 labels 2026-03-17 03:06:03 +00:00
hamza.khyari force-pushed feature/post-resource-types-virtual from 3e4c2e8286 to 1a7673cb92 2026-03-17 03:06:57 +00:00 Compare
hamza.khyari force-pushed feature/post-resource-types-virtual from 1a7673cb92 to ff1c1ed6f6 2026-03-17 03:19:47 +00:00 Compare
hamza.khyari force-pushed feature/post-resource-types-virtual from ff1c1ed6f6 to a73307e0aa 2026-03-17 03:40:37 +00:00 Compare
brent.edwards requested changes 2026-03-17 04:52:55 +00:00
Dismissed
brent.edwards left a comment
Member
Copy Link
Copy Source

PR #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_types reference types from #662

None 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_NAMES
This 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.py
This PR inlines 3 type dicts after *DATABASE_TYPE_DEFS. PR #661 adds *VIRTUAL_CORE_TYPES at 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 (checking criteria, 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.py will be the bottleneck for future additions.

Suggestion: Extract to _resource_registry_virtual_deferred.py for consistency, or add the 3 types to #661's _resource_registry_virtual.py if merge order permits.

6. Drive-by docstring cleanup
Removing the implementation_plan.md reference from resource_registry_service.py is 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 naming
PR #661 uses "criteria" in its equivalence dicts. The PR description here uses identity_key and identity_fields terminology. The actual code uses "criteria". The naming should be consistent with whatever the equivalence tracking service (#334) will expect. If #334 expects identity_key/identity_fields, the data structures here need to match.

8. Validation could use structural typing

if "criteria" not in self.equivalence:
    ...
criteria = self.equivalence["criteria"]
if not isinstance(criteria, list) or len(criteria) == 0:

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 current dict[str, Any] pattern.

P3 — Observations

  • YAML configs correct: resource_kind: virtual, sandbox_strategy: none, user_addable: false, capabilities all false, handler: null/None.
  • Equivalence criteria are well-chosen: url for remote, url+path for submodule, name+target_path for symlink.
  • Commit message: feat(resource): add deferred virtual resource types — correct.
  • Labels and milestone: correct.
  • Test coverage: 42 behave scenarios, 6 robot tests, 10 ASV benchmarks — thorough for 3 types.
  • resource_type.py at 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)

Order PR Reason
1st #662 (physical) Introduces types referenced by #661 and #663; rewrites BUILTIN_NAMES
2nd #661 (virtual core) References #662 child types; independent of #663
3rd #663 (virtual deferred) References #662 child types; enhanced validation benefits from #661 existing

After each merge, the next PR must rebase to resolve conflicts in _resource_registry_data.py, resource_type.py, and resource_registry_service.py.

## PR #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_types` reference types from #662** - `remote` → `child_types: ["git-remote"]` — introduced by #662 - `submodule` → `child_types: ["git-submodule"]` — introduced by #662 - `symlink` → `child_types: ["fs-symlink", "git-tree-entry"]` — both introduced by #662 None 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_NAMES`** This 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.py`** This PR inlines 3 type dicts after `*DATABASE_TYPE_DEFS`. PR #661 adds `*VIRTUAL_CORE_TYPES` at 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` (checking `criteria`, `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.py` will be the bottleneck for future additions. **Suggestion:** Extract to `_resource_registry_virtual_deferred.py` for consistency, or add the 3 types to #661's `_resource_registry_virtual.py` if merge order permits. **6. Drive-by docstring cleanup** Removing the `implementation_plan.md` reference from `resource_registry_service.py` is 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 naming** PR #661 uses `"criteria"` in its equivalence dicts. The PR description here uses `identity_key` and `identity_fields` terminology. The actual code uses `"criteria"`. The naming should be consistent with whatever the equivalence tracking service (#334) will expect. If #334 expects `identity_key`/`identity_fields`, the data structures here need to match. **8. Validation could use structural typing** ```python if "criteria" not in self.equivalence: ... criteria = self.equivalence["criteria"] if not isinstance(criteria, list) or len(criteria) == 0: ``` 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 current `dict[str, Any]` pattern. --- ### P3 — Observations - YAML configs correct: `resource_kind: virtual`, `sandbox_strategy: none`, `user_addable: false`, capabilities all false, `handler: null`/`None`. - Equivalence criteria are well-chosen: `url` for remote, `url+path` for submodule, `name+target_path` for symlink. - Commit message: `feat(resource): add deferred virtual resource types` — correct. - Labels and milestone: correct. - Test coverage: 42 behave scenarios, 6 robot tests, 10 ASV benchmarks — thorough for 3 types. - `resource_type.py` at 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) | Order | PR | Reason | |-------|----|--------| | 1st | **#662** (physical) | Introduces types referenced by #661 and #663; rewrites BUILTIN_NAMES | | 2nd | **#661** (virtual core) | References #662 child types; independent of #663 | | 3rd | **#663** (virtual deferred) | References #662 child types; enhanced validation benefits from #661 existing | After each merge, the next PR must rebase to resolve conflicts in `_resource_registry_data.py`, `resource_type.py`, and `resource_registry_service.py`.
src/cleveragents/application/services/_resource_registry_data.py Outdated
@@ -196,6 +196,92 @@ BUILTIN_TYPES: list[dict[str, Any]] = [
},
},
*DATABASE_TYPE_DEFS,
# === Deferred Virtual Resource Types (#331) ===
brent.edwards commented 2026-03-17 04:52:55 +00:00
Member
Copy Link
Copy Source

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.py for consistency with the sibling PRs.

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.py` for consistency with the sibling PRs.
src/cleveragents/application/services/_resource_registry_data.py Outdated
@@ -199,0 +207,4 @@
"sandbox_strategy": "none",
"user_addable": False,
"built_in": True,
"cli_args": [],
brent.edwards commented 2026-03-17 04:52:55 +00:00
Member
Copy Link
Copy Source

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. Add Depends on #662 to the PR description.

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. Add `Depends on #662` to the PR description.
src/cleveragents/domain/models/core/resource_type.py Outdated
@@ -310,3 +311,1 @@
f"Virtual resource type '{self.name}' requires an "
"'equivalence' configuration for deduplication."
)
# Virtual types require equivalence rules with criteria and have
brent.edwards commented 2026-03-17 04:52:55 +00:00
Member
Copy Link
Copy Source

P1: 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.

P1: 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.
brent.edwards requested changes 2026-03-17 04:59:38 +00:00
Dismissed
brent.edwards left a comment
Member
Copy Link
Copy Source

Code Review — PR #663 feat(resource): add deferred virtual resource types

Reviewer: @brent.edwards | Size: M (116 new lines) | Focus: Equivalence metadata, type taxonomy

P1:must-fix (2)

1. child_types reference types that only exist after #662 merges
symlink declares child_types: ["fs-symlink", "git-tree-entry"]. Both types are introduced by PR #662. remote → git-remote and submodule → git-submodule have the same dependency. If #663 merges before #662, bootstrap registration fails or creates types with dangling child references.
Fix: Add Depends on: #662 to 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 call bootstrap_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. symlink type declares child_types: ["fs-symlink", "git-tree-entry"] — but git-tree-entry is 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 virtual symlink type. 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.py at 486 lines — comfortable margin for now.

Positive Observations

  • Equivalence metadata is well-structured with clear identity_key/identity_fields
  • 42 scenarios — thorough coverage for 3 types
  • YAML configs are consistent with the #661 pattern

Verdict: REQUEST_CHANGES — merge dependency (P1-1) and auto-bootstrap removal impact (P1-2) need resolution.

## Code Review — PR #663 `feat(resource): add deferred virtual resource types` **Reviewer:** @brent.edwards | **Size:** M (116 new lines) | **Focus:** Equivalence metadata, type taxonomy --- ### P1:must-fix (2) **1. `child_types` reference types that only exist after #662 merges** `symlink` declares `child_types: ["fs-symlink", "git-tree-entry"]`. Both types are introduced by PR #662. `remote` → `git-remote` and `submodule` → `git-submodule` have the same dependency. If #663 merges before #662, bootstrap registration fails or creates types with dangling child references. **Fix:** Add `Depends on: #662` to 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 call `bootstrap_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.** `symlink` type declares `child_types: ["fs-symlink", "git-tree-entry"]` — but `git-tree-entry` is 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 virtual `symlink` type. 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.py` at 486 lines — comfortable margin for now. --- ### Positive Observations - Equivalence metadata is well-structured with clear identity_key/identity_fields - 42 scenarios — thorough coverage for 3 types - YAML configs are consistent with the #661 pattern **Verdict:** REQUEST_CHANGES — merge dependency (P1-1) and auto-bootstrap removal impact (P1-2) need resolution.
hamza.khyari force-pushed feature/post-resource-types-virtual from a73307e0aa to 8a016a48f8 2026-03-17 12:13:30 +00:00 Compare
hamza.khyari commented 2026-03-17 12:13:58 +00:00
Author
Member
Copy Link
Copy Source

Response 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_types is a declarative list[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 calls self.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 in ResourceRegistryService.__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_TYPES in _resource_registry_data.py. This matches the pattern from #661 (_resource_registry_virtual.py / VIRTUAL_CORE_TYPES). _resource_registry_data.py is now 399 lines (down from 480).

P1-6: Drive-by docstring cleanup — NOTED

The implementation_plan.md reference 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-entry by mode=120000 (symlink mode) before creating links. This is a #664 concern, not a type definition issue — the child_types declaration 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 referenced identity_key/identity_fields which has been corrected.

P3-8: Structural typing for equivalence — NOTED

Agreed a nested EquivalenceSpec Pydantic model would be better than dict[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.

## Response 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_types` is a declarative `list[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 calls `self.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 in `ResourceRegistryService.__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_TYPES` in `_resource_registry_data.py`. This matches the pattern from #661 (`_resource_registry_virtual.py` / `VIRTUAL_CORE_TYPES`). `_resource_registry_data.py` is now 399 lines (down from 480). ### P1-6: Drive-by docstring cleanup — NOTED The `implementation_plan.md` reference 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-entry` by mode=120000 (symlink mode) before creating links. This is a #664 concern, not a type definition issue — the `child_types` declaration 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 referenced `identity_key`/`identity_fields` which has been corrected. ### P3-8: Structural typing for equivalence — NOTED Agreed a nested `EquivalenceSpec` Pydantic model would be better than `dict[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`.
aditya commented 2026-03-17 13:49:45 +00:00
Member
Copy Link
Copy Source

Code Review — PR #663

  • Merge readiness: REQUEST_CHANGES

This 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 create remote/submodule/symlink resources; and (2) on current master these 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

ID Severity Category Finding Evidence Required Action Type
F-001 P1 Functional correctness / Spec compliance Deferred virtual types are still manually addable at runtime, violating the requirement to keep them non-user-addable and hidden from add scaffolding. src/cleveragents/application/services/_resource_registry_ops.py register_resource() persists any existing type without checking ResourceTypeModel.user_addable. Runtime repro on this branch source (PYTHONPATH=src): register_resource(type_name='remote'/'submodule'/'symlink', ...) succeeds (ADDED ...). Enforce user_addable in service/CLI path (prefer service-level guard): reject manual creation when type_row.user_addable is False unless call path is internal auto-discovery. Add explicit negative tests for these three types via service and CLI. Code
F-002 P1 Integration dependency / Merge safety New deferred virtual types reference child types not present on current master; PR still requires strict dependency sequencing. Runtime on fresh in-memory bootstrap (branch source): remote -> ['git-remote'] missing ['git-remote'], submodule -> ['git-submodule'] missing ['git-submodule'], symlink -> ['fs-symlink','git-tree-entry'] missing ['fs-symlink','git-tree-entry']. Keep this PR blocked on physical-types dependency and rebase after dependency merge. Ensure Forgejo dependency direction is correct (PR blocks issue; issue depends on PR) and merge order is enforced in PR metadata. PR/Process
F-003 P2 Test quality / False negatives New tests validate schema/domain shape but do not validate the core runtime contract that deferred virtual types cannot be manually registered. features/resource_type_deferred_virtual.feature and robot/resource_type_deferred_virtual.robot include no scenario/case asserting register_resource('remote'...) fails. Existing tests all pass while runtime allows add. Add BDD + Robot negative tests that attempt manual add and assert a validation failure message. This should fail before fix and pass after fix. Code
F-004 P2 Validation rigor New virtual-type validation accepts invalid equivalence criteria elements (non-string / empty string), weakening guarantees for identity metadata. src/cleveragents/domain/models/core/resource_type.py: checks only criteria exists and is non-empty list. Repro: ResourceTypeSpec.from_config(... equivalence={'criteria':[123]}) and [''] both pass. Tighten validation: require criteria to be a non-empty list[str] with each entry non-empty and trimmed; add negative tests for invalid item types/values. Code
F-005 P3 PR metadata accuracy PR narrative claims do not fully match current code/test state (review noise risk). In PR details: mentions 7 Robot tests and extracted _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. Update PR description/comments to reflect actual current implementation and test counts. PR/Process

Consolidated Checklist

  • P1
    • Block manual registration for non-user-addable deferred virtual types (remote, submodule, symlink) in runtime path.
    • Add regression tests proving manual add is rejected (unit + integration level).
    • Keep merge order/dependency enforcement explicit until missing child types exist in base branch.
  • P2
    • Add runtime-focused test coverage for add-path behavior (not only schema/domain loading).
    • Strengthen equivalence.criteria validation to list[str] with non-empty entries.
  • P3
    • Correct PR description mismatches (test counts / extraction claim) to match actual branch content.

Fix Plan

  1. Fastest safe path: add guard in register_resource() to reject user_addable=False types for manual calls.
  2. Add focused BDD and Robot negative tests for manual add rejection of remote/submodule/symlink.
  3. Tighten ResourceTypeSpec criteria item validation and add negative tests for non-string/empty entries.
  4. Re-run targeted tests.
  5. Update PR metadata text to reflect current code and explicitly keep dependency sequencing.

Dependencies:

  • Step 2 depends on Step 1 (tests should assert the new guard).
  • Step 4 depends on Steps 1-3.
  • Process update (Step 5) can be done anytime before re-review.
## Code Review — PR #663 - Merge readiness: `REQUEST_CHANGES` This 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 create `remote`/`submodule`/`symlink` resources; and (2) on current `master` these 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 | ID | Severity | Category | Finding | Evidence | Required Action | Type | |---|---|---|---|---|---|---| | F-001 | **P1** | Functional correctness / Spec compliance | Deferred virtual types are still manually addable at runtime, violating the requirement to keep them non-user-addable and hidden from add scaffolding. | `src/cleveragents/application/services/_resource_registry_ops.py` `register_resource()` persists any existing type without checking `ResourceTypeModel.user_addable`. Runtime repro on this branch source (`PYTHONPATH=src`): `register_resource(type_name='remote'/'submodule'/'symlink', ...)` succeeds (`ADDED ...`). | Enforce `user_addable` in service/CLI path (prefer service-level guard): reject manual creation when `type_row.user_addable is False` unless call path is internal auto-discovery. Add explicit negative tests for these three types via service and CLI. | `Code` | | F-002 | **P1** | Integration dependency / Merge safety | New deferred virtual types reference child types not present on current `master`; PR still requires strict dependency sequencing. | Runtime on fresh in-memory bootstrap (branch source): `remote -> ['git-remote'] missing ['git-remote']`, `submodule -> ['git-submodule'] missing ['git-submodule']`, `symlink -> ['fs-symlink','git-tree-entry'] missing ['fs-symlink','git-tree-entry']`. | Keep this PR blocked on physical-types dependency and rebase after dependency merge. Ensure Forgejo dependency direction is correct (PR blocks issue; issue depends on PR) and merge order is enforced in PR metadata. | `PR/Process` | | F-003 | **P2** | Test quality / False negatives | New tests validate schema/domain shape but do not validate the core runtime contract that deferred virtual types cannot be manually registered. | `features/resource_type_deferred_virtual.feature` and `robot/resource_type_deferred_virtual.robot` include no scenario/case asserting `register_resource('remote'...)` fails. Existing tests all pass while runtime allows add. | Add BDD + Robot negative tests that attempt manual add and assert a validation failure message. This should fail before fix and pass after fix. | `Code` | | F-004 | **P2** | Validation rigor | New virtual-type validation accepts invalid equivalence criteria elements (non-string / empty string), weakening guarantees for identity metadata. | `src/cleveragents/domain/models/core/resource_type.py`: checks only `criteria` exists and is non-empty list. Repro: `ResourceTypeSpec.from_config(... equivalence={'criteria':[123]})` and `['']` both pass. | Tighten validation: require `criteria` to be a non-empty `list[str]` with each entry non-empty and trimmed; add negative tests for invalid item types/values. | `Code` | | F-005 | **P3** | PR metadata accuracy | PR narrative claims do not fully match current code/test state (review noise risk). | In PR details: mentions 7 Robot tests and extracted `_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`. | Update PR description/comments to reflect actual current implementation and test counts. | `PR/Process` | ### Consolidated Checklist - **P1** - [ ] Block manual registration for non-user-addable deferred virtual types (`remote`, `submodule`, `symlink`) in runtime path. - [ ] Add regression tests proving manual add is rejected (unit + integration level). - [ ] Keep merge order/dependency enforcement explicit until missing child types exist in base branch. - **P2** - [ ] Add runtime-focused test coverage for add-path behavior (not only schema/domain loading). - [ ] Strengthen `equivalence.criteria` validation to `list[str]` with non-empty entries. - **P3** - [ ] Correct PR description mismatches (test counts / extraction claim) to match actual branch content. ### Fix Plan 1. **Fastest safe path:** add guard in `register_resource()` to reject `user_addable=False` types for manual calls. 2. Add focused BDD and Robot negative tests for manual add rejection of `remote`/`submodule`/`symlink`. 3. Tighten `ResourceTypeSpec` criteria item validation and add negative tests for non-string/empty entries. 4. Re-run targeted tests. 5. Update PR metadata text to reflect current code and explicitly keep dependency sequencing. Dependencies: - Step 2 depends on Step 1 (tests should assert the new guard). - Step 4 depends on Steps 1-3. - Process update (Step 5) can be done anytime before re-review.
freemo commented 2026-03-17 18:39:00 +00:00
Owner
Copy Link
Copy Source

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:

  • @hamza.khyari — Address F-001 (enforce user_addable guard) and F-003 (negative tests). Push fix commit.
  • Merge order: #662 → #661 → #663.

PM status — Day 37

## 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**: - @hamza.khyari — Address F-001 (enforce `user_addable` guard) and F-003 (negative tests). Push fix commit. - Merge order: #662 → #661 → #663. *PM status — Day 37*
brent.edwards commented 2026-03-17 19:50:35 +00:00
Member
Copy Link
Copy Source

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_types referenced 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.py module comment also documents the #662 dependency. Clear.

P1-2 (Constructor auto-bootstrap removed without migration path): RESOLVED
resource_registry_service.py line 105 shows self.bootstrap_builtin_types() is called in __init__ with the try/except fallback for pre-migration scenarios. The PR body explicitly states "Auto-bootstrap in ResourceRegistryService.__init__ is preserved (NOT removed)". Confirmed in the code.

P3-4 (resource_type.py at 486 lines): RESOLVED
resource_type.py is now 476 lines. The resource_registry_service.py is 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.yaml still has child_types: ["fs-symlink", "git-tree-entry"] where git-tree-entry covers 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: criteria must exist, must be a list, must be non-empty; user_addable must be false; sandbox_strategy must be none; handler must be None; 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

Finding Severity Status
P1-1 child_types forward refs / merge dep P1 Resolved
P1-2 Auto-bootstrap removed P1 Resolved
P2-3 symlink → git-tree-entry scope P3 Accepted (downgraded)
P3-4 resource_type.py 486 lines P3 Resolved
NEW: YAML spec references missing P3 Open
NEW: Validation merge-order sensitivity P2 Open (mitigated by stated order)

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 (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_types` referenced 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.py` module comment also documents the #662 dependency. Clear. **P1-2 (Constructor auto-bootstrap removed without migration path): RESOLVED** `resource_registry_service.py` line 105 shows `self.bootstrap_builtin_types()` is called in `__init__` with the try/except fallback for pre-migration scenarios. The PR body explicitly states "Auto-bootstrap in `ResourceRegistryService.__init__` is preserved (NOT removed)". Confirmed in the code. **P3-4 (resource_type.py at 486 lines): RESOLVED** `resource_type.py` is now 476 lines. The `resource_registry_service.py` is 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.yaml` still has `child_types: ["fs-symlink", "git-tree-entry"]` where `git-tree-entry` covers 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: `criteria` must exist, must be a list, must be non-empty; `user_addable` must be false; `sandbox_strategy` must be `none`; `handler` must be `None`; 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 | Finding | Severity | Status | |---------|----------|--------| | P1-1 child_types forward refs / merge dep | P1 | Resolved | | P1-2 Auto-bootstrap removed | P1 | Resolved | | P2-3 symlink → git-tree-entry scope | P3 | Accepted (downgraded) | | P3-4 resource_type.py 486 lines | P3 | Resolved | | NEW: YAML spec references missing | P3 | **Open** | | NEW: Validation merge-order sensitivity | P2 | **Open** (mitigated by stated order) | **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.
brent.edwards approved these changes 2026-03-17 19:53:37 +00:00
Dismissed
brent.edwards left a comment
Member
Copy Link
Copy Source

Re-Review — PR #663 feat(resource): add deferred virtual resource types

Reviewer: @brent.edwards | Round 2 — checking resolution of 4 prior findings

Prior Findings Status

# Sev Finding Status
P1-1 child_types reference types from #662 RESOLVED — PR body documents Depends on: #662 and recommended merge order (#662 → #661 → #663)
P1-2 Constructor auto-bootstrap removed RESOLVED — bootstrap_builtin_types() confirmed still in __init__
P2-3 symlink → git-tree-entry too broad Accepted as P3 — design decision, mode 120000 filtering is the equivalence service's responsibility (#334)
P3-4 resource_type.py at 486 lines RESOLVED — now 476 lines

New Findings (2)

P3: YAML files missing # Spec reference: comments — #661's YAMLs now have them, but #663's remote.yaml, submodule.yaml, symlink.yaml do 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.

## Re-Review — PR #663 `feat(resource): add deferred virtual resource types` **Reviewer:** @brent.edwards | **Round 2** — checking resolution of 4 prior findings --- ### Prior Findings Status | # | Sev | Finding | Status | |---|-----|---------|--------| | P1-1 | child_types reference types from #662 | **RESOLVED** — PR body documents `Depends on: #662` and recommended merge order (#662 → #661 → #663) | | P1-2 | Constructor auto-bootstrap removed | **RESOLVED** — `bootstrap_builtin_types()` confirmed still in `__init__` | | P2-3 | symlink → git-tree-entry too broad | **Accepted as P3** — design decision, mode 120000 filtering is the equivalence service's responsibility (#334) | | P3-4 | resource_type.py at 486 lines | **RESOLVED** — now 476 lines | ### New Findings (2) **P3: YAML files missing `# Spec reference:` comments** — #661's YAMLs now have them, but #663's `remote.yaml`, `submodule.yaml`, `symlink.yaml` do 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.
hamza.khyari force-pushed feature/post-resource-types-virtual from 8a016a48f8 to dc72b79715 2026-03-18 01:58:51 +00:00 Compare
hamza.khyari dismissed brent.edwards's review 2026-03-18 01:58:52 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

hamza.khyari force-pushed feature/post-resource-types-virtual from dc72b79715 to 7a2182505f 2026-03-18 02:07:07 +00:00 Compare
hamza.khyari commented 2026-03-18 02:08:02 +00:00
Author
Member
Copy Link
Copy Source

Response 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-116 already rejects types with user_addable=False:

if not type_row.user_addable:
    raise ValidationError(
        f"Resource type '{type_name}' is not user-addable. ..."
    )

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:

  • "manual register_resource rejects remote as not user-addable"
  • "manual register_resource rejects submodule as not user-addable"
  • "manual register_resource rejects symlink as not user-addable"

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 in criteria must be a str and must be non-empty after stripping whitespace. Added 2 new negative test scenarios:

  • "equivalence with non-string criteria element fails validation" (passes [123])
  • "equivalence with empty-string criteria element fails validation" (passes [""])

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-24631 at 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.

## Response 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-116` already rejects types with `user_addable=False`: ```python if not type_row.user_addable: raise ValidationError( f"Resource type '{type_name}' is not user-addable. ..." ) ``` 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: - "manual register_resource rejects remote as not user-addable" - "manual register_resource rejects submodule as not user-addable" - "manual register_resource rejects symlink as not user-addable" 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 in `criteria` must be a `str` and must be non-empty after stripping whitespace. Added 2 new negative test scenarios: - "equivalence with non-string criteria element fails validation" (passes `[123]`) - "equivalence with empty-string criteria element fails validation" (passes `[""]`) **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-24631` at 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`.
hamza.khyari force-pushed feature/post-resource-types-virtual from 7a2182505f to 5d6cb099ad 2026-03-18 02:30:08 +00:00 Compare
hamza.khyari merged commit ab1fd19bcd into master 2026-03-18 02:40:32 +00:00
hamza.khyari deleted branch feature/post-resource-types-virtual 2026-03-18 02:40:38 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
freemo
Labels
Clear labels
auto/needs-reevaluation
Controller deferred this PR; awaiting Phase 6+ scope-evaluator or operator re-enablement.
 controller-managed
Auto-agents controller manages this PR/issue (see tools/controller/deploy/RUNBOOK.md). Remove this label to abandon controller management.
 overdue
auto/blocked-by-deps
PR blocked by an open issue dependency. Operator must close the dep (or remove the dependency link) before the merge driver can act. Auto-cleared by merge_drive when no open deps remain.
 auto/ci-timeout
Most recent merge cycle hit CI timeout. Driver excludes this PR while last merge_cycle row is < 30 min old; label persists thereafter as visible history.
 auto/claimed-implementer
Currently being processed by an implementer worker.
 auto/claimed-merge
Currently being processed by the merge driver.
 auto/claimed-reviewer
Currently being processed by a reviewer worker.
 auto/driver-down
Merge driver heartbeat stale; pipeline halted. Closed automatically on next clean tick.
 auto/invariant-violation
Detected master commit violating the strict merge invariant. Tracked as an issue (not a PR label); kept here for label completeness.
 auto/last-attempt-tier-0
In-cycle escalation: most recent attempt ran at the Tier 0 slot (`tier-0`). Slot's model defined in .opencode/models/tiers.yaml.
 auto/last-attempt-tier-1
In-cycle escalation: most recent attempt ran at the Tier 1 slot (`tier-1`). Slot's model defined in .opencode/models/tiers.yaml.
 auto/last-attempt-tier-2
In-cycle escalation: most recent attempt ran at the Tier 2 slot (`tier-2`). Slot's model defined in .opencode/models/tiers.yaml. Gated behind IMPLEMENTER_ESCALATION_TIER2_ENABLED.
 auto/last-attempt-tier-min
In-cycle escalation: most recent attempt ran at the Tier -1 slot (`tier-min`). Slot's model defined in .opencode/models/tiers.yaml. Suffix is ``-min`` (not ``--1``) so the Forgejo UI reads naturally.
 Automation Tracking
Tracking issues used by the AI Automation system for agents to communicate and report.
 auto/needs-conflict-resolution
Rebase conflict needs LLM conflict-resolver.
 auto/needs-implementer
Failing CI needs implementer attention.
 auto/postmortem
Documenting a driver incident or rollback.
 auto/ready-to-merge
Reviewer has APPROVED this PR and no later REQUEST_CHANGES is outstanding. The merge driver requires this label to even consider a PR for merging. Set by the reviewer worker on APPROVE; cleared on REQUEST_CHANGES.
 auto/restart-throttled
Train repeatedly lost master-tempo races. Driver excludes via merge_cycle until cooldown elapses; label persists as visible history.
 auto/revert
Revert PR backing out an invariant violation. Fast-tracked through the merge driver.
 auto/sentinel
Sentinel PR duplicated from upstream into a personal fork by tools/duplicate_prs_to_fork.py for pipeline testing. Lives only in the fork; the canonical pipeline never sees it.
 auto/stale-inactivity
No implementer activity for N days. Flagged for human review. Auto-cleared on next push to head branch.
 auto/unstable
Repeatedly fails on current master (>= 3 ci-fail-on-rebased-sha releases in 12 h). Excluded from driver until human triage.
 Blocked
A ticket in a blocked state and unable to complete until some other task is completed first.
Bounty
$100
A bounty of $100 for any open-source contributor who provides a MR that solves this issue
Bounty
$1000
A bounty of $1000 for any open-source contributor who provides a MR that solves this issue
Bounty
$10000
A bounty of $10000 for any open-source contributor who provides a MR that solves this issue
Bounty
$20
A bounty of $20 for any open-source contributor who provides a MR that solves this issue
Bounty
$2000
A bounty of $2000 for any open-source contributor who provides a MR that solves this issue
Bounty
$250
A bounty of $250 for any open-source contributor who provides a MR that solves this issue
Bounty
$50
A bounty of $50 for any open-source contributor who provides a MR that solves this issue
Bounty
$500
A bounty of $500 for any open-source contributor who provides a MR that solves this issue
Bounty
$5000
A bounty of $5000 for any open-source contributor who provides a MR that solves this issue
Bounty
$750
A bounty of $750 for any open-source contributor who provides a MR that solves this issue
MoSCoW
Could have
Could have feature in order to satisfy the epic/legendary.
MoSCoW
Must have
Must have feature in order to satisfy the epic/legendary.
MoSCoW
Should have
Should have feature in order to satisfy the epic/legendary.
Needs Feedback
There are questions in the ticket that can not be completed until the project owner provides clarity.
Points
1
1 man-hours worth of work for an expert with no learning curve.
Points
13
13 man-hours worth of work for an expert with no learning curve.
Points
2
2 man-hours worth of work for an expert with no learning curve.
Points
21
21 man-hours worth of work for an expert with no learning curve.
Points
3
3 man-hours worth of work for an expert with no learning curve.
Points
34
34 man-hours worth of work for an expert with no learning curve.
Points
5
5 man-hours worth of work for an expert with no learning curve.
Points
55
55 man-hours worth of work for an expert with no learning curve.
Points
8
8 man-hours worth of work for an expert with no learning curve.
Points
88
88 man-hours worth of work for an expert with no learning curve.
Priority
Backlog
This ticket has backlogged priority and is not to be worked on yet
Priority
CI Blocker
Critical priority issue that blocks CI/CD pipeline and prevents PR merges
Priority
Critical
The priority is critical
Priority
High
The priority is high
Priority
Low
The priority is low
Priority
Medium
The priority is medium
Signed-off: Owner
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.
 Signed-off: Scrum Master
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.
 Signed-off: Tech Lead
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.
 Spike
A ticket for learning a tool or technology that is needed to be able to do future planning and design.
State
Completed
The ticket has been fully implemented, completed, and merged with the source code. This label should only be applied once a ticket is closed.
State
Duplicate
A ticket that represents the same content as an existing ticket.
State
In Progress
A ticket that is actively being developed.
State
In Review
A ticket that has had some code completed to implement but is waiting to pass peer review and is not yet merged in.
State
Paused
This ticket's work started but wasn't finished. It's on hold (likely in a feature branch) and will be resumed later, either due to a blocker or a delay.
State
Unverified
All new tickets start in this state. A developer may set it to show the ticket is unverified. This means we haven't agreed to work on it. It will either move to a verified state or be closed as wontdo.
State
Verified
The issue has been verified by a developer as legitimate. It will be worked on and verified tickets are now considered part of the backlog.
State
Wont Do
This ticket has been decided it wont be done. This may mean the bug has been determined to not be real (cant verify) or the feature is one we have decided we dont want to adopt.
Type
Automation
Any edits or discussion about the AI automated coding system.
Type
Bug
Something that doesnt work as intended.
Type
Discussion
Anytime a ticket represents a discussion about a subject and doesnt fall into one of the other categories.
Type
Documentation
An error or improvement needed in the documentation.
Type
Epic
Any first tier epic. That is, an epic which contains only issues as children and will not have sub-epics.
Type
Feature
Some new functionality not present.
Type
Legendary
A type of Epic which will contain other Epics.
Type
Refactor
A code change that restructures existing code without changing its external behavior.
Type
Support
Someone needs help using the project.
Type
Task
A generic task that doesnt fit into the other type categories.
Type
Testing
Work exclusively focusing on fixing or expanding testing.
No Label
State
In Review
Type
Feature
Milestone
No items
No Milestone v3.6.0
Projects
Clear projects
No project
Assignees
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
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
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?