feat(acms): add scoped backend view filtering #607
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
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
cleveragents/cleveragents-core!607
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature/m6-acms-scoped-view"
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
Implement project-resource isolation for the ACMS context pipeline (Forgejo issue #193).
Changes
ResourceScope: Frozen Pydantic model holding resource ULIDs, project names, temporal scope, and include/exclude path globs (matched viaPurePath.full_match()for recursive**support)ScopedBackendView: Wraps text/vector/graph backends to auto-injectscopeparameter; filtersTieredFragmentvisibility by project + resource ID; supportsdenied_resource_idsdenylistScopedBackendSet: Bundles scoped views for all three backend typesResourceAliasResolver: Translates user-facing aliases to canonical ULIDs with uniqueness validationresolve_resource_scope(): BuildsResourceScopefrom projects with allowlist/denylist filteringvalidate_project_scope()/validate_resource_scope(): Guards raisingScopeViolationErrorfor out-of-scope accessContextTierServiceenforcement hooks:get_scoped_by_resource,validate_fragment_scope,store_with_scope_checkScopedBackendViewintiers.pyreplaced with re-export fromscoped_view.pyfor backward compatibilityFiles Changed (11)
src/cleveragents/domain/models/acms/scoped_view.pysrc/cleveragents/domain/models/acms/__init__.pysrc/cleveragents/domain/models/acms/tiers.pysrc/cleveragents/application/services/context_tiers.pyfeatures/scoped_view.featurefeatures/steps/scoped_view_steps.pyrobot/scoped_view.robotrobot/helper_scoped_view.pybenchmarks/scoped_view_bench.pydocs/reference/scoped_backend_view.mdCHANGELOG.mdQuality Gates
Spec References
docs/specification.mdlines 42501-42566 (Scoped Views)docs/specification.mdlines 3760-3790 (CLI flags)docs/specification.mdlines 43030-43065 (InitialContextAssembler)Closes #193
hamza.khyari referenced this pull request2026-03-05 13:48:14 +00:00
5ce8ac53bd9f0993dce29f0993dce2b100dc530cCode Review -- PR #607
feat(acms): add scoped backend view filteringReviewed commit:
c146de5(latest on branch)Issue: #193
Spec reference:
docs/specification.mdlines 42501--42566Overall Assessment
The feature implementation is solid -- the
ResourceScope,ScopedBackendView,ScopedBackendSet,ResourceAliasResolver, and tier enforcement hooks are well-structured, correctly use frozen Pydantic v2 models, and follow the project's domain-driven patterns. ThePrivateAttr+model_post_initcaching in the latest commit is the correct approach for frozen models and addresses a real hot-path performance concern.The test suite is thorough (67 Behave scenarios, 8 Robot tests, ASV benchmarks). However, I found issues across three categories that should be addressed before merge.
Requesting changes due to one HIGH-severity spec compliance gap and two MEDIUM-severity security concerns.
Findings Summary
resolve_resource_scope()is_visible()bypasses resource-level check for fragments with emptyresource_idContextTierService.get()bypasses all scope enforcementstore_with_scope_checkemptyproject_namebypass (documented in this commit)from_resource_scope()doesn't forwarddenied_resource_ids(by design, but undocumented)list/setbut nottuplesearch_vector/search_graphdenied-scope tests_coerce_allowed_projectslist-input testeffective_scope()resolve()ScopedBackendViewobject construction per tier-service calleffective_scope()caching is well-implementedSPEC1 -- Missing DAG Traversal [HIGH]
Spec reference:
docs/specification.mdlines 42517--42521The specification explicitly defines step 3 of Resource Scope Resolution as:
resolve_resource_scope()only collects directlinked_resourcesfrom projects and does not perform DAG traversal to include child resources. Agit-checkoutresource with auto-discovered children (e.g.,fs-directory,devcontainer-instance) would be excluded from the scope unless explicitly linked.Action required: Either implement DAG expansion (accepting a registry lookup callback), or add explicit documentation noting this as a known limitation with a TODO referencing the Resource Registry integration.
S1 --
is_visible()Emptyresource_idBypass [MEDIUM]When
resource_idsis populated on the view (resource-level isolation active), a fragment withresource_id=""and a validproject_namepasses visibility without any resource-level gate. Combined withstore_with_scope_checkalso skipping the resource check for emptyresource_id, such a fragment can be both stored and read even when strict resource-level filtering is intended.No test covers this edge case (T1).
Action required: Either reject fragments with empty
resource_idwhenresource_idsis populated, or add a test scenario explicitly documenting this as intentional behavior for resource-agnostic fragments.S2 --
ContextTierService.get()Has No Scope Enforcement [MEDIUM]The
get()method retrieves any fragment by ID from any tier without scope filtering. If this method is reachable from actor tool bindings (via the DI container), it constitutes a scope bypass vector -- an actor knowing a fragment ID from another project can retrieve it directly.Action required: Audit that
get()is not exposed to actors, or add aget_scoped()variant requiring aResourceScopeparameter.Issue #193 Remaining Subtasks
Two subtasks are still unchecked in the issue:
nox -s coverage_reportnox(all default sessions) and fix any failuresThese must be completed before the issue can be closed.
See inline comments on specific code locations below.
[S2 -- MEDIUM / Security] This method retrieves a fragment by ID from any tier without any scope enforcement. If this method is reachable from actor tool bindings (e.g., exposed through the DI container or a tool wrapper), an actor could retrieve fragments belonging to other projects by guessing or obtaining fragment IDs.
The scoped methods (
get_scoped_view,get_scoped_by_resource,get_for_actor) all apply filtering, but this method does not.Suggestion: Either (a) rename to
_getto signal internal-only use, (b) add aget_scoped(fragment_id, scope)variant, or (c) document that this method is internal and must not be exposed to actor tool bindings.@ -169,3 +190,3 @@if not project_names:return []scoped = ScopedBackendView(allowed_projects=project_names)scoped = ScopedBackendView([P2 -- LOW / Performance] Each call to
get_scoped_view,get_scoped_by_resource,get_scoped_metrics, andvalidate_fragment_scopeconstructs a newScopedBackendViewobject. For high-frequency call sites, consider accepting a pre-builtScopedBackendViewas a parameter or caching it per plan/request lifecycle.@ -0,0 +222,4 @@v: frozenset[str] | list[str] | set[str],) -> frozenset[str]:"""Accept list/set and convert to frozenset."""if isinstance(v, (list, set)):[B2 -- LOW / Logic] The three
_coerce_*validators handlelistandsetbut nottuple. A caller passingtuple(["R1", "R2"])would rely on Pydantic v2's implicit coercion rather than this explicit validator. Consider addingtupleto the isinstance check for consistency:@ -0,0 +312,4 @@if resource_id:return self.is_resource_in_scope(resource_id)return True[S1 -- MEDIUM / Security] When
resource_idsis populated on this view but a fragment hasresource_id="", this line returnsTruewithout any resource-level check. A fragment withproject_name="proj-a"andresource_id=""passes bothis_visible()andstore_with_scope_check(), bypassing resource-level isolation.Either:
if self.resource_ids and not resource_id: return Falsebefore this line to enforce resource-level filtering when active, or@ -0,0 +422,4 @@A new ``ScopedBackendView`` configured with the scope'sresource IDs and project names."""return cls([B1 -- LOW / Logic] This factory method does not forward
denied_resource_idsto the view. This is correct becauseresolve_resource_scope()already subtracts denied resources fromresource_idsbefore building theResourceScope. However, this subtle design decision is not documented here and could confuse future maintainers who see thedenied_resource_idsfield on the view but never see it populated via this path.Suggestion: Add a brief comment, e.g.:
@ -0,0 +781,4 @@for project in projects:project_names.add(project.namespaced_name)for lr in project.linked_resources:[SPEC1 -- HIGH / Spec Compliance] The specification (
docs/specification.mdlines 42517--42521) requires step 3 of scope resolution to expand to child resources via DAG traversal:This loop only collects direct
linked_resources. Child resources in the Resource Registry DAG (e.g., auto-discoveredfs-directorychildren of agit-checkout) are not included.Suggestion: Accept an optional
registry_lookup: Callable[[str], frozenset[str]] | Noneparameter that, when provided, expands each resource ID to include its descendants. Default toNonefor backward compatibility.Review: feat(acms): add scoped backend view filtering
Reviewed per
docs/development/review_playbook.md. Routing: domain models & services → primary Luis, secondary Jeff; security → primary Jeff, secondary Brent.Luis's prior
REQUEST_CHANGESraised several important issues. After full analysis, I confirm the most critical ones remain unresolved. This PR has 3 P0 security/correctness blockers that must be fixed before merge.P0:blocker — Scope bypass in
is_visible()whenresource_idsis populatedWhen
ScopedBackendView.resource_idsis populated (i.e., ULID-level filtering is active),is_visible()only checksis_resource_in_scope()for fragments that have a non-emptyresource_id. Fragments with an emptyresource_idsilently pass through and returnTrue, bypassing the resource-level scope entirely.This means any fragment without a
resource_idattribute (or withresource_id="") that belongs to an allowed project will be visible regardless of the resource scope. This is an information leakage vector in multi-project environments.Fix: When
self.resource_idsis populated, fragments with emptyresource_idshould be excluded (returnFalse), not silently allowed. The safe default is deny-by-default for resource-scoped views.File:
src/cleveragents/domain/models/acms/scoped_view.py,is_visible()method. Confirms Luis's S1 finding.P0:blocker — No scope enforcement on
ContextTierService.get()direct ID lookupContextTierService.get(fragment_id)retrieves any fragment by ID without scope validation. An actor with a scoped view could callget()with a known fragment ID from another project and retrieve it, bypassing project isolation entirely.While
get_scoped_view()andget_scoped_by_resource()enforce scope, the rawget()method is the most natural lookup path and has no guardrails.Fix: Either add a
scope-awareget()overload that validates before returning, or documentget()as internal-only and add a scoped wrapper. Confirms Luis's S2 finding.File:
src/cleveragents/application/services/context_tiers.py,get()method.P0:blocker —
PurePath.full_match()requires Python 3.12+ResourceScope.matches_path()usesPurePath.full_match()which was added in Python 3.12 (see CPython changelog). If the project supports Python 3.11 or earlier, this will raiseAttributeErrorat runtime.Fix: Use
PurePath.match()(available since 3.4) with appropriate adjustments for the matching semantics, or gate behind a version check. Checkpyproject.tomlrequires-pythonto confirm minimum version.File:
src/cleveragents/domain/models/acms/scoped_view.py,matches_path()method.P1:must-fix — File exceeds 500-line limit
scoped_view.pyis 819 lines which exceeds the project's 500-line single-file limit (per CONTRIBUTING.md). This is a process blocker.Fix: Extract
ResourceAliasResolverand/orresolve_resource_scope()+ validation functions into a separatescope_resolution.pymodule.P1:must-fix —
denied_resource_idsnot forwarded infrom_resource_scope()factoryScopedBackendView.from_resource_scope(scope)creates the view withresource_ids=scope.resource_idsandallowed_projects=scope.project_names, but does NOT forwarddenied_resource_ids. Any resources in the scope's exclude list will not be denied in the resulting view.Fix: The factory should also pass
denied_resource_idsderived fromscope.exclude_resourcesafter alias resolution. Confirms Luis's B1 finding.P1:must-fix — Missing DAG child-resource expansion per spec §42517-42521
The spec requires that when a resource is in scope, its DAG children are also included. The current implementation does flat ULID-set membership checking with no graph traversal.
Fix: This may be a future-PR item, but if the spec requires it for this milestone, it needs implementation. Confirms Luis's SPEC1 finding.
P2:should-fix
resource_idsaccepts any string, allowing typos or SQL injection payloads in future backends. Add afield_validatorwith ULID regex.search_text(query="")andsearch_vector(embedding=[], top_k=0)are accepted without validation.tuplenot coerced in field validators — The_coerce_*validators handlelistandsetbut nottuple. Passing atuplefalls through to thereturn vbranch which expectsfrozenset. Confirms Luis's B2.ScopedBackendViewconstruction —get_for_actor(),get_scoped_view(),get_scoped_metrics()all construct newScopedBackendViewinstances with the same project names. Consider accepting a pre-built view.__all__intiers.py— The re-export module lacks__all__.Process notes
mergeable: false). Rebase needed.REQUEST_CHANGESshould be refreshed — several of his findings are confirmed here.The scoped view concept is well-designed and the test coverage (42 BDD + 8 Robot) is thorough. However, the P0 security bypasses in
is_visible()andget(), plus the Python 3.12 compatibility issue, must be resolved before this can merge.P0:blocker —
get()retrieves any fragment by ID without scope enforcement. An actor with a scoped view can bypass project isolation by callingget(fragment_id)with a known ID from another project.Consider adding a
get_scoped(fragment_id, scope)method that validates the fragment against the scope before returning, or makeget()accept an optionalscopeparameter.@ -0,0 +152,4 @@Whether the path passes the filters."""pp = PurePath(path)if self.include_paths and not any(P0:blocker —
PurePath.full_match()requires Python 3.12+. This will raiseAttributeErroron Python 3.11 and earlier.Use
PurePath.match()instead (available since 3.4), or add a runtime version check. Note thatmatch()anchors differently (matches from the right by default), so test cases may need adjustment.@ -0,0 +244,4 @@v: frozenset[str] | list[str] | set[str],) -> frozenset[str]:"""Accept list/set and convert to frozenset."""if isinstance(v, (list, set)):P0:blocker — Scope bypass: when
self.resource_idsis populated, fragments with emptyresource_idsilently pass through and returnTrue.The
if resource_id:guard means any fragment without aresource_idbypasses ULID-level filtering entirely. Whenresource_idsis set, the safe default should be deny for fragments missingresource_id.@ -0,0 +314,4 @@return True# -- Backend-level query proxying ----------------------------------------P1:must-fix —
from_resource_scope()does not forwarddenied_resource_idsfrom the scope. Resources in the scope's exclude list will not be denied in the constructed view.Note:
ResourceScope.exclude_resourcescontains aliases/names, not ULIDs, so alias resolution is needed here. Alternatively, ensure the caller resolves excludes into theresource_idsset before calling this factory.679c0823ad4d9cfee4f9Addressed Review Findings from @brent.edwards (Review #1991)
All findings addressed in commit
4d9cfee4.P0:blocker — Scope bypass in
is_visible()✅ Fixedis_visible()now returnsFalsefor fragments with emptyresource_idwhenresource_idsis populated (deny-by-default). Added 2 Behave scenarios covering both cases.P0:blocker — No scope enforcement on
get()✅ FixedAdded
get_scoped(fragment_id, scope)method toContextTierServicethat validates fragments against scope before returning. ReturnsNoneand logs a warning for out-of-scope fragments. Added 3 Behave scenarios.P0:blocker —
PurePath.full_match()requires Python 3.12+ ✅ Not applicablepyproject.tomlspecifiesrequires-python = ">=3.13".PurePath.full_match()was added in Python 3.12, so this is not a compatibility issue. Added a clarifying docstring note.P1:must-fix — File exceeds 500-line limit ✅ Fixed
Split
scoped_view.py(830→473 lines) by extractingResourceAliasResolver,validate_project_scope,validate_resource_scope,resolve_resource_scopeinto newscope_resolution.py(325 lines).P1:must-fix —
denied_resource_idsnot forwarded ✅ DocumentedAdded docstring note to
from_resource_scope()explaining thatdenied_resource_idsis not forwarded becauseresolve_resource_scope()already subtracts denied IDs fromresource_idsbefore constructing theResourceScope.P1:must-fix — Missing DAG child-resource expansion ✅ Fixed
Added optional
registry_lookup: Callable[[str], frozenset[str]] | Noneparameter toresolve_resource_scope()for DAG expansion per spec §42517-42521. Defaults toNonefor backward compatibility. Added 1 Behave scenario.P2:should-fix items ✅ Fixed
tupletoisinstancechecks in all three_coerce_*validators. Added 1 Behave scenario.__all__intiers.py: Added.Process notes
master(fae438a7).Summary of changes
scoped_view.pyis_visible()fix, P1 B1 docstring, P2 tuple coercion, trimmed to 473 linesscope_resolution.pycontext_tiers.pyget_scoped()method, updated importstiers.py__all__added__init__.pyscoped_view.featurescoped_view_steps.pybenchmarks/,robot/,docs/4d9cfee4f9d428c39601Round 2 Verification Review —
feat(acms): add scoped backend view filteringReviewed commit:
4d9cfee4(latest on branch)Base:
fae438a7(merge base with master)Verification of Round 1 Findings
All P0 and P1 blockers from review #1991 are confirmed resolved.
is_visible()scope bypassscoped_view.py:276-277— fragments with emptyresource_idnow returnFalsewhenresource_idsis populated (deny-by-default). Docstring updated. 2 new BDD scenarios cover both cases (feature lines 421-429).get()scope bypasscontext_tiers.py:139-176— newget_scoped(fragment_id, scope)validates viaScopedBackendView.from_resource_scope()+is_visible()before returning. ReturnsNoneand logs warning for out-of-scope. 3 new BDD scenarios (feature lines 432-465).PurePath.full_match()compatpyproject.tomlrequires>=3.13.full_match()requires>=3.12. Docstring now notes the dependency explicitly (scoped_view.py:128-129).scoped_view.pytrimmed to 474 lines.scope_resolution.pyextracted at 326 lines. Both under 500.denied_resource_idsnot forwardedfrom_resource_scope()docstring (scoped_view.py:338-345) explains thatresolve_resource_scope()subtracts denied IDs before constructingResourceScope. Verified:scope_resolution.py:294-299doesall_resource_ids -= denied.resolve_resource_scope()now accepts `registry_lookup: Callablestr], frozenset[str_coerce_*validators now checkisinstance(v, (list, set, tuple)). 1 new scenario.__all__intiers.pytiers.py:268-276— 7 symbols exported.Quality Gates
nox -s lintnox -s typechecknox -s unit_tests -- features/scoped_view.featureRemaining Findings (non-blocking)
Three P2 and one P3 items noted below as inline comments. None are blockers — these can be addressed in a follow-up commit or deferred.
Approved. The security fixes in
is_visible()and the newget_scoped()method correctly close the scope bypass vectors. The file split, DAG expansion, and documentation improvements are all well-executed. Good work addressing the Round 1 feedback, Hamza.@ -12,0 +21,4 @@allowlist/denylist filtering. `validate_project_scope()` and`validate_resource_scope()` guard against out-of-scope access. Three enforcementhooks added to `ContextTierService`: `get_scoped_by_resource`,`validate_fragment_scope`, `store_with_scope_check`. Includes 67 Behave BDDP2:should-fix — CHANGELOG says "67 Behave BDD scenarios" (line 24) but the actual count is 74 scenarios (verified via
nox -s unit_tests). The response comment also says 74. Update the count to match.This was 67 before the review-fix commit added 7 new scenarios for the P0/P1/P2 fixes.
@ -0,0 +37,4 @@| Field | Type | Description ||-------|------|-------------|| `resource_ids` | `frozenset[str]` | Resource ULIDs in scope (min 1) |P2:should-fix — Three items in this doc are stale after the file split and DAG expansion:
Line 40:
resource_idsdescription says "min 1" but the code allowsfrozenset()(empty) — see feature line 27 "creating a scope with no resources should succeed" andscoped_view.py:66which has nomin_lengthconstraint. Remove the "min 1" claim.Lines 153-161:
resolve_resource_scope()signature is missing the newregistry_lookupparameter added for DAG expansion.Lines 256-261: Module Locations table lists
scoped_viewas "Core implementation (all types and functions)" butscope_resolution.pyis now a separate module containingResourceAliasResolver,validate_project_scope,validate_resource_scope, andresolve_resource_scope. Add a row forscope_resolution.py.@ -0,0 +1,1163 @@"""Step definitions for scoped backend view filtering.P3:nit — This file is 1164 lines, well over the 500-line guideline. Step definition files tend to grow with scenario count and may warrant more latitude, but consider splitting into
scoped_view_steps.py(core view/scope steps) andscoped_view_tier_steps.py(ContextTierService steps) if the file grows further.@ -121,6 +136,45 @@ class ContextTierService:return Nonedef get_scoped(P2:should-fix — This file is now 568 lines, up from 365 on the base branch. CONTRIBUTING.md (line 396) says "Keep files under 500 lines." The 203-line growth from this PR pushed it over.
Consider extracting the scope-enforcement methods (
get_scoped,get_scoped_by_resource,validate_fragment_scope,store_with_scope_check,get_scoped_metrics) into aScopedTierServicemixin or a separatescoped_tiers.pymodule. This would also improve separation of concerns between core tier management and scope enforcement.Approved.
New commits pushed, approval review dismissed automatically according to repository settings
5521f22bbdb028c80cab