feat(account-management): add in-process Tenant Resolver plugin#14
Open
diffora wants to merge 1 commit into
Open
feat(account-management): add in-process Tenant Resolver plugin#14diffora wants to merge 1 commit into
diffora wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (14)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (10)
📝 WalkthroughWalkthroughThis PR adds an in-process Tenant Resolver plugin to Account Management: new crate module ChangesTenant Resolver Plugin
Sequence Diagram(s)sequenceDiagram
participant Client
participant PluginImpl
participant DB
participant TypesRegistry
Client->>PluginImpl: TenantResolver API call
PluginImpl->>DB: read tenants / tenant_closure
DB-->>PluginImpl: rows
PluginImpl->>TypesRegistry: resolve tenant_type(s)
TypesRegistry-->>PluginImpl: type schema(s)
PluginImpl-->>Client: TenantResolver response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/system/account-management/account-management/src/module.rs`:
- Around line 466-484: The TR plugin registration must be made idempotent and
the client must be bound before the type is advertised: before calling
types_registry.register(...) ensure ctx.client_hub().register_scoped::<dyn
TenantResolverPluginClient>(ClientScope::gts_id(&tr_instance_id), tr_api) is
performed so the scoped client is available first; then call
types_registry.register and handle AlreadyExists results by treating an
AlreadyExists as success only if the existing registered spec equals the
serialized tr_instance (otherwise surface an error) — you can do this by
querying the registry for tr_instance_id (or inspecting tr_results for
AlreadyExists and comparing stored spec) and only failing
RegisterResult::ensure_all_ok when the existing spec differs. Ensure you
reference tr_instance_id, tr_instance_json /
BaseModkitPluginV1::<TenantResolverPluginSpecV1>, types_registry.register,
RegisterResult::ensure_all_ok, ctx.client_hub().register_scoped, and
tr_api/tr_plugin when making the changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: db6cf4c5-8178-4c70-833b-ac12c39ae1c1
⛔ Files ignored due to path filters (1)
tools/dylint_lints/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
modules/system/account-management/account-management/Cargo.tomlmodules/system/account-management/account-management/src/lib.rsmodules/system/account-management/account-management/src/module.rsmodules/system/account-management/account-management/src/tr_plugin/error_map.rsmodules/system/account-management/account-management/src/tr_plugin/mod.rsmodules/system/account-management/account-management/src/tr_plugin/plugin_impl.rsmodules/system/account-management/account-management/src/tr_plugin/projection.rsmodules/system/account-management/account-management/src/tr_plugin/queries.rsmodules/system/account-management/account-management/src/tr_plugin/tests.rs
diffora
force-pushed
the
am/06-tr-plugin-clean
branch
from
May 9, 2026 19:24
ded999b to
c6d83d5
Compare
diffora
commented
May 9, 2026
diffora
commented
May 9, 2026
diffora
commented
May 9, 2026
diffora
commented
May 9, 2026
diffora
commented
May 9, 2026
diffora
commented
May 9, 2026
diffora
commented
May 9, 2026
diffora
left a comment
diffora
left a comment
Owner
Author
There was a problem hiding this comment.
Rust PR Review — PR #14: in-process Tenant Resolver plugin
Overall: The implementation is well-structured, extensively documented, and the test suite is meaningfully behavioral. The module boundary (co-location in AM), registration ordering, error taxonomy, and provisioning-invisibility invariants are all handled correctly. The comments explaining design decisions (graph-vs-emission split, cycle protection, let _ = conn rationale) are above average.
Six findings were posted as inline comments. Summary:
| # | ID | Sev | Location | Issue |
|---|---|---|---|---|
| 1 | MODKIT-SEC-002 / RUST-SEC-001 | HIGH | queries.rs:22 |
AccessScope::allow_all() constructed directly — bypasses PolicyEnforcer contract without a designated escalated-trust path |
| 2 | RUST-ERR-001 / RUST-NO-002 | MEDIUM | queries.rs:336 |
let _ = conn drop semantics are opaque — correctness relies on DbConn not being Clone/Arc-backed; invariant not documented |
| 3 | RUST-PERF-001 | MEDIUM | queries.rs:488 |
get_descendants bulk-hydrates the full subtree for type resolution even when max_depth is small; no follow-up ticket or cap |
| 4 | RUST-TYPE-001 | LOW | queries.rs:348 |
get_ancestors sorts by tenants.depth (root-relative) without documenting that invariant; fragile if AM ever allows reparenting |
| 5 | RUST-TEST-001 | MEDIUM | tests.rs:167 |
Missing test: get_tenants with empty status vs explicit [Active,Suspended,Deleted] — does not exercise the is_empty() branch of tenants_status_in_condition |
| 6 | RUST-TEST-001 | MEDIUM | tests.rs:472 |
Missing test: is_ancestor where both endpoints exist but have no closure row (unrelated siblings) — the count > 0 terminal branch is untested |
Framework compliance: The module layout, SDK pattern (tr_plugin stays pub(crate), SDK trait lives in tenant-resolver-sdk), ClientHub registration, and GTS plugin registration order are all correct. The types-registry-sdk dev-dependency is properly scoped. No REST layer, no migrations, no drift.
Test suite assessment: 31 tests, all behavioral (no constructor echos, no tautologies). The provisioning invisibility and barrier semantics coverage is thorough. The unimplemented! panic guards on unused registry methods are a good pattern. Two gaps noted above (items 5 and 6).
Finding #1 (MODKIT-SEC-002) is the only framework-level concern and is worth resolving before merge if the framework offers an internal()/trust_gateway() constructor. If it does not, a documented exception is acceptable. Findings #2, #5, and #6 are the most actionable low-effort fixes.
diffora
force-pushed
the
am/06-tr-plugin-clean
branch
4 times, most recently
from
May 9, 2026 20:53
8b07a62 to
ff8e772
Compare
diffora
commented
May 10, 2026
diffora
commented
May 10, 2026
diffora
commented
May 10, 2026
diffora
commented
May 10, 2026
diffora
left a comment
diffora
left a comment
Owner
Author
There was a problem hiding this comment.
Follow-up Review — PR #14 pass 2
Squashed commit ff8e772. Verifying all 6+1 prior findings, reporting 3 new LOW findings.
Prior Findings — All Resolved
| # | Finding | Status |
|---|---|---|
| 1 | MODKIT-SEC-002: AccessScope::allow_all() called directly at 8 sites | FIXED — plugin_scope() wrapper centralizes and documents the trust elevation |
| 2 | RUST-ERR-001/NO-002: let _ = conn drops without explanation | FIXED — both drop sites now carry single-owner-contract comments |
| 3 | RUST-PERF-001: full subtree type-hydration before max_depth trim | ACKNOWLEDGED in module doc with recursive-CTE follow-up note; no regression |
| 4 | RUST-TYPE-001: get_ancestors depth ordering undocumented | FIXED — comment explains root-relative depth and reparenting caveat |
| 5 | RUST-TEST-001: missing get_tenants empty-status vs explicit-all test | FIXED — get_tenants_empty_status_equals_all_visible added |
| 6 | RUST-TEST-001: missing is_ancestor unrelated-siblings test | FIXED — is_ancestor_unrelated_siblings_returns_false added |
| CR | Registration ordering + AlreadyExists idempotency | FIXED — register_scoped before types_registry.register; spec-compare on AlreadyExists |
New Findings (3 LOW — documentation only)
| # | ID | Sev | Location | Issue |
|---|---|---|---|---|
| 1 | RUST-DOC-001 | LOW | plugin_impl.rs:30 | types_registry field doc says "failures degrade to None" — actual behavior is TenantResolverError::Internal |
| 2 | RUST-DOC-001 | LOW | queries.rs:36 (module doc) | Same stale "degrades to None" claim in the module-level bullet |
| 3 | RUST-STYLE-001 | LOW | queries.rs get_tenants | conn held live across resolve_tenant_types_for_rows without a drop or comment; inconsistent with the explicit-drop pattern in get_ancestors / get_descendants |
Verdict: Ready to merge. No correctness, security, or test gaps remain.
diffora
force-pushed
the
am/06-tr-plugin-clean
branch
from
May 10, 2026 06:48
ff8e772 to
4c2a2ef
Compare
diffora
commented
May 10, 2026
diffora
commented
May 10, 2026
diffora
left a comment
diffora
left a comment
Owner
Author
There was a problem hiding this comment.
Follow-up Review — PR #14 pass 3
Current HEAD: 4c2a2ef. Verifying all pass-2 findings and inspecting the specific pass-3 changes.
Pass-2 Findings — All Resolved
| # | Finding | Status |
|---|---|---|
| 1 | RUST-DOC-001: plugin_impl.rs:30 — field doc said "degrades to None" |
FIXED — doc now correctly says TenantResolverError::Internal |
| 2 | RUST-DOC-001: queries.rs module doc — stale "degrades to None" claim |
FIXED — module doc now says "fails the SDK call with TenantResolverError::Internal; the plugin must not return raw UUIDs" |
| 3 | RUST-STYLE-001: get_tenants held conn live inconsistently |
FIXED — explicit let _ = conn; with single-owner-contract comment added, matching get_ancestors / get_descendants pattern |
Pass-3 Changes — Verification
config.rs — TrPluginConfig with enabled: bool default false
Correct. #[serde(default, deny_unknown_fields)] present. Custom impl Default with enabled = false and priority = 1000 is sound. validate() requires no new checks for these fields (all i16 values are valid priorities, bool needs no range check).
module.rs — if tr_plugin_enabled gate + ordering
The registration ordering (types-registry THEN ClientHub bind) is correct and intentional: a registry failure cannot leave a stale ClientHub entry. The module-level doc accurately describes why. The AlreadyExists idempotency check via get_instance + JSON comparison is correct: the entity is a GTS instance, so get_instance is the right retrieval method.
module.rs — else branch logs "disabled by config"
Correct. The log message includes the opt-in instruction (set account-management.tr_plugin.enabled = true), which is operationally helpful.
plugin_impl.rs / queries.rs / projection.rs — doc updates
All three updated correctly. No stale "degrades to None" claims remain.
config_tests.rs — new pinning tests
tr_plugin_disabled_by_default: sound pinning test. The assertion !cfg.tr_plugin.enabled will fail when the switch-over commit flips the default, which is the stated intent.
tr_plugin_priority_default_loses_to_in_tree_alternatives: sound pinning test. priority > 100 correctly pins that AM loses to both in-tree alternatives (rg-tr-plugin = 50, static-tr-plugin = 100) since > 100 implies > 50 transitively.
New Finding (pass 3)
| # | ID | Sev | Location | Issue |
|---|---|---|---|---|
| 1 | RUST-DOC-001 | LOW | module.rs:70 (and module doc lines 25-32) | "tenant-resolver" dep described as "purely an init-order constraint" — does not mention unconditional binary-presence requirement |
Detail: The deps = [...] declaration is at macro / binary-topology level, not config level. Any binary that ships AM without tenant-resolver will fail modkit init at startup even when tr_plugin.enabled = false — the framework never reaches AM's init() code to evaluate the flag. The existing comment says "purely an init-order constraint" which is technically true for the enabled = true path but omits the enabled = false consequence. An inline comment suggestion has been posted.
Verdict
All pass-2 findings resolved. One new LOW documentation finding posted as an inline comment. No correctness, security, async, or test gaps introduced by the pass-3 changes. Ready to merge.
diffora
force-pushed
the
am/06-tr-plugin-clean
branch
from
May 10, 2026 07:01
4c2a2ef to
53a7e62
Compare
diffora
commented
May 10, 2026
diffora
force-pushed
the
am/06-tr-plugin-clean
branch
from
May 10, 2026 07:18
53a7e62 to
67264f9
Compare
diffora
commented
May 10, 2026
diffora
commented
May 10, 2026
diffora
force-pushed
the
am/06-tr-plugin-clean
branch
from
May 10, 2026 07:37
67264f9 to
ee3b030
Compare
diffora
commented
May 10, 2026
diffora
left a comment
diffora
left a comment
Owner
Author
There was a problem hiding this comment.
Follow-up Review — PR #14 pass 6
Current HEAD: ee3b030. Verifying pass-5 findings and reviewing new changes.
Pass-5 Findings — Verification
| # | Finding | Status |
|---|---|---|
| 1 | RUST-OBS-001: audit_barrier_bypass fired before visibility probe in is_ancestor |
FIXED — audit now emits after the visibility+self-reference checks |
| 2 | RUST-TEST-001: missing symmetric corrupt-closure test for get_descendants |
FIXED — corrupt_closure_to_provisioning_descendant_yields_internal added |
| 3 | RUST-DOC-001: inst-flow-get-root-tenant-return-service-unavailable-none/-multiple step IDs embed stale name |
PARTIAL — step bodies updated to say Internal; ID token strings still carry service-unavailable |
Pass-6 Changes — Verification
get_descendants graph walk fail-closes on revisit/missing-row
Correct. visited.insert(node_id) now returns Err(Internal) on a cycle (was warn+continue). The missing-row branch inside the loop is the defense-in-depth case for a race between the pre-walk hydration check and the actual walk. Both correctly surface Internal.
Post-walk completeness check (only when max_depth = None)
Correct in structure. The walked set removes the pivot before comparing to closure_population (which excludes the pivot by construction). Skipping the check when max_depth is bounded is intentional and documented. Two new failure paths lack test coverage — see inline comments.
tr_plugin.vendor: String config field
Correctly defaulting to "cyberfabric" and wired into tr_instance.vendor. The deny_unknown_fields + serde(default) attributes are in place. No validation enforces non-empty; an empty vendor would register silently and never be selectable. See inline comment.
audit_barrier_bypass() placement
All three methods (is_ancestor, get_ancestors, get_descendants) now fire the audit AFTER the visibility probe. Correct.
New corrupt_closure_to_provisioning_descendant_yields_internal test
Tests the hydrated_ids != closure_ids path in get_descendants. Sound and behavioral.
DESIGN.md / PRD.md / feature doc updates
All documentation changes are consistent with the implementation. Read-only role deferral is clearly framed, audit warn pinned, structural enforcement documented.
New Findings (pass 6)
| # | ID | Sev | Location | Issue |
|---|---|---|---|---|
| 1 | RUST-TEST-001 | MEDIUM | queries.rs (get_descendants walk) |
Cycle-detection and post-walk completeness branches both untested |
| 2 | RUST-TEST-001 | LOW | config_tests.rs |
No test pins tr_plugin.vendor default value |
| 3 | RUST-DOC-001 | LOW | feature-tenant-resolver-plugin.md |
Stale service-unavailable step IDs carried forward (partial fix of pass-5 finding) |
Verdict: No new correctness, security, or async issues introduced. Two test gaps (one MEDIUM) on the new fail-closed walk paths. Ready to merge if the cycle-detection test gap is acceptable given the provisioning-corruption test already exercising the adjacent hydration-mismatch path.
diffora
commented
May 10, 2026
diffora
commented
May 10, 2026
diffora
force-pushed
the
am/06-tr-plugin-clean
branch
3 times, most recently
from
May 10, 2026 08:06
2d0187d to
c1acd5b
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/system/account-management/docs/tr-plugin/features/feature-tenant-resolver-plugin.md (1)
156-163:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove
InvalidArgumentbranches from plugin flows — malformed payloads cannot reach the typed SDK boundary.The documented flows at lines 156–163 and 218–225 model handling of malformed
statusvalues or negativemax_depth, but the SDK types prevent these cases entirely:statusisVec<TenantStatus>(a closed enum with Active, Suspended, Deleted only), andmax_depthisOption<u32>(unsigned). These invalid-input branches belong in gateway/transport validation docs, not in the plugin trait flow itself. Remove them to eliminate the self-contradiction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/system/account-management/docs/tr-plugin/features/feature-tenant-resolver-plugin.md` around lines 156 - 163, Remove the invalid-input branches from the plugin flow docs: delete the steps that return CanonicalError::InvalidArgument for malformed `GetTenantsOptions` (the branches under the get_tenants(ids, options) flow that check unknown `status` and negative `max_depth`) since `TenantStatus` is a closed enum and `max_depth` is an Option<u32> at the SDK boundary; keep the database/internal error branch that returns TenantResolverError::Internal and ensure remaining steps still reference `ClientHub`, propagation of `SecurityContext` and OpenTelemetry trace context, and the inst-flow-* identifiers (e.g., inst-flow-get-tenants-receive, inst-flow-get-tenants-return-validation) are removed or adjusted so the flow no longer documents InvalidArgument handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/system/account-management/account-management/src/tr_plugin/tests.rs`:
- Around line 186-191: The test setup uses connect_db with
ConnectOpts::default() which leaves max_conns at 10 causing separate in-memory
SQLite databases per pooled connection; update the setup() call to constrain the
pool to a single connection (e.g., build ConnectOpts with max_conns: Some(1)) or
switch the DSN to a named shared-cache URI (e.g.,
sqlite:file:am_test?mode=memory&cache=shared) and still set max_conns: Some(1)
so that run_migrations_for_testing(&db, Migrator::migrations()) and subsequent
queries use the same in-memory database.
In `@modules/system/account-management/docs/tr-plugin/DESIGN.md`:
- Line 80: The intra-doc anchor referenced from the
cpt-cf-tr-plugin-fr-get-descendants row (text in PluginImpl::get_descendants)
points to a non-existent fragment (`#34-non-functional-requirements-allocation`);
fix it by making the link fragment and the heading match: either change the link
to the heading's slug (likely `#nfr-allocation` or `#nfr-allocation-subtree-latency`
depending on how other anchors are generated) or change the "#### NFR
Allocation" heading to include the expected fragment/id (e.g., add an explicit
"34. NFR Allocation" or an HTML anchor) so the reference to §3.4 / subsection
`subtree-latency` resolves correctly; update any nearby textual label (`§3.4 NFR
subtree-latency`) to use the same fragment name.
In
`@modules/system/account-management/docs/tr-plugin/features/feature-tenant-resolver-plugin.md`:
- Around line 336-338: Update step 4 (tagged inst-algo-preorder-emit-preorder)
to reflect the current implementation: state that preorder is produced by an
application-layer in-memory pre-order walk over the parent map returned from a
single non-recursive tenant_closure scan (with max_depth applied during the
walk), and that the prior claim of emitting preorder directly from a single SQL
query is a future optimization gated on modkit-db exposing raw ConnectionTrait /
a recursive-CTE hook; keep the note that direct DB emission will be implemented
once the recursive CTE path in inst-algo-preorder-walk-tenants /
inst-algo-preorder-join-closure becomes available.
- Around line 419-435: The DoD and acceptance criteria are inconsistent about
read-only-role enforcement (item id
cpt-cf-tr-plugin-dod-tenant-resolver-plugin-read-only-role-enforcement); update
the acceptance criteria to match the deferred `p2` stance: remove any current
requirement for startup/CI privilege assertions and DB-side write-rejection,
state that when tr_plugin.enabled = true the plugin emits a startup audit
warning (target = "am.tr_plugin.audit") and that full enforcement will be
implemented once modkit-db exposes a per-role connection pool (see DESIGN §3.5
and constraint-read-only-role) — alternatively, if you intended immediate
enforcement, change the DoD back to `p1` and implement the startup/CI privilege
checks and bootstrap failure logic now instead of deferring.
In `@modules/system/account-management/docs/tr-plugin/PRD.md`:
- Around line 661-662: The PRD currently has conflicting statements: the
paragraph about deferred "AM-owned read-only role provisioning" (mentions
modkit-db, per-role connection pool, SELECT grants on tenants and
tenant_closure, plugin asserting grants at boot) is marked deferred, but the
acceptance criteria still require startup/CI grant checks and DB-side write
rejection as present-PR outcomes; update the acceptance criteria to match the
deferred status by rephrasing those ACs into future-state language (e.g., "will
verify", "to be implemented in follow-up") or move them into a clearly labeled
follow-up item so the document is internally consistent with the deferred plan
and references to AM and plugin behavior.
- Around line 624-625: The PRD currently contradicts itself about how status
filtering is applied for get_descendants: one section claims it's a SQL
predicate on tenant_closure.descendant_status while another describes an
in-memory emission predicate during the preorder walk; pick the latter to match
the implementation. Update all occurrences so the single canonical story states
that get_descendants applies status filtering as an in-memory emission predicate
during the deterministic pre-order walk (and does not filter the starting
tenant), whereas get_tenants applies status filtering as a SQL predicate on
tenants.status; also update acceptance criteria and tests references
(get_descendants, get_tenants, tenant_closure, tenants, and the preorder walk
description) to reflect this consistent behavior.
---
Outside diff comments:
In
`@modules/system/account-management/docs/tr-plugin/features/feature-tenant-resolver-plugin.md`:
- Around line 156-163: Remove the invalid-input branches from the plugin flow
docs: delete the steps that return CanonicalError::InvalidArgument for malformed
`GetTenantsOptions` (the branches under the get_tenants(ids, options) flow that
check unknown `status` and negative `max_depth`) since `TenantStatus` is a
closed enum and `max_depth` is an Option<u32> at the SDK boundary; keep the
database/internal error branch that returns TenantResolverError::Internal and
ensure remaining steps still reference `ClientHub`, propagation of
`SecurityContext` and OpenTelemetry trace context, and the inst-flow-*
identifiers (e.g., inst-flow-get-tenants-receive,
inst-flow-get-tenants-return-validation) are removed or adjusted so the flow no
longer documents InvalidArgument handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c0bc2c2-fd7b-4545-878f-3d895aca0d3c
⛔ Files ignored due to path filters (1)
tools/dylint_lints/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
modules/system/account-management/account-management/Cargo.tomlmodules/system/account-management/account-management/src/config.rsmodules/system/account-management/account-management/src/config_tests.rsmodules/system/account-management/account-management/src/lib.rsmodules/system/account-management/account-management/src/module.rsmodules/system/account-management/account-management/src/tr_plugin/error_map.rsmodules/system/account-management/account-management/src/tr_plugin/mod.rsmodules/system/account-management/account-management/src/tr_plugin/plugin_impl.rsmodules/system/account-management/account-management/src/tr_plugin/projection.rsmodules/system/account-management/account-management/src/tr_plugin/queries.rsmodules/system/account-management/account-management/src/tr_plugin/tests.rsmodules/system/account-management/docs/tr-plugin/DESIGN.mdmodules/system/account-management/docs/tr-plugin/PRD.mdmodules/system/account-management/docs/tr-plugin/features/feature-tenant-resolver-plugin.md
✅ Files skipped from review due to trivial changes (1)
- modules/system/account-management/account-management/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (7)
- modules/system/account-management/account-management/src/tr_plugin/mod.rs
- modules/system/account-management/account-management/src/lib.rs
- modules/system/account-management/account-management/src/tr_plugin/error_map.rs
- modules/system/account-management/account-management/src/tr_plugin/projection.rs
- modules/system/account-management/account-management/src/tr_plugin/plugin_impl.rs
- modules/system/account-management/account-management/src/module.rs
- modules/system/account-management/account-management/src/tr_plugin/queries.rs
diffora
force-pushed
the
am/06-tr-plugin-clean
branch
from
May 10, 2026 08:23
c1acd5b to
746d7f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
modules/system/account-management/docs/tr-plugin/PRD.md (1)
624-625:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign
get_descendantsmechanics across all sections (still contradictory).These lines now define current behavior as in-memory pre-order walk + in-memory descendant status emission filtering, but §6.2 (Line 377) and §8.4 Main Flow (Line 560) still describe current behavior as recursive CTE + SQL descendant-status filtering. Please make one canonical “current vs target” story across PRD sections so acceptance tests/NFR validation don’t diverge.
Also applies to: 666-666
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/system/account-management/docs/tr-plugin/PRD.md` around lines 624 - 625, The PRD currently contradicts itself: update all references so they present one canonical "current vs target" story for get_descendants/get_ancestors/get_tenants — state explicitly that the current implementation of get_descendants is an in-memory deterministic pre-order walk over a parent map built from the barrier-bounded tenant_closure scan with descendant status filtering applied at emission (not during closure scan), get_ancestors returns rows ordered by tenants.depth DESC, tenants.id, and get_tenants applies status filtering as a SQL predicate on tenants.status; state the target/desired approach separately as using a recursive CTE (the recursive-CTE target shape) with SQL-level descendant-status filtering if that is the intended future change. Then make the same wording changes to the checklist bullets (the two lines in the diff) and the descriptions in §6.2 and §8.4 (and the other noted location) so every section consistently labels which behavior is "current" (in-memory walk + emission filter) and which is "target" (recursive CTE + SQL filter).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/system/account-management/docs/tr-plugin/PRD.md`:
- Line 628: Update the Risks §12 entry to reflect that startup grant assertion
and CI role checks are deferred (future-state) rather than current mitigations
for the tr_plugin read-only contract: change the mitigation text to
“future-state: startup grant assertion + CI role checks” or split the mitigation
into “current” (startup audit warning under target = "am.tr_plugin.audit" and
structural read-only enforcement via find()/count() and absence of
secure_insert/secure_update/secure_delete) and “deferred/future” (database role
grants and CI startup assertion per DESIGN §3.5
cpt-cf-tr-plugin-constraint-read-only-role); also apply the same wording fix to
the corresponding row referenced near line 661.
---
Duplicate comments:
In `@modules/system/account-management/docs/tr-plugin/PRD.md`:
- Around line 624-625: The PRD currently contradicts itself: update all
references so they present one canonical "current vs target" story for
get_descendants/get_ancestors/get_tenants — state explicitly that the current
implementation of get_descendants is an in-memory deterministic pre-order walk
over a parent map built from the barrier-bounded tenant_closure scan with
descendant status filtering applied at emission (not during closure scan),
get_ancestors returns rows ordered by tenants.depth DESC, tenants.id, and
get_tenants applies status filtering as a SQL predicate on tenants.status; state
the target/desired approach separately as using a recursive CTE (the
recursive-CTE target shape) with SQL-level descendant-status filtering if that
is the intended future change. Then make the same wording changes to the
checklist bullets (the two lines in the diff) and the descriptions in §6.2 and
§8.4 (and the other noted location) so every section consistently labels which
behavior is "current" (in-memory walk + emission filter) and which is "target"
(recursive CTE + SQL filter).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bd2a9a45-5820-41ef-b252-64ef499b705e
⛔ Files ignored due to path filters (1)
tools/dylint_lints/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
modules/system/account-management/account-management/Cargo.tomlmodules/system/account-management/account-management/src/config.rsmodules/system/account-management/account-management/src/config_tests.rsmodules/system/account-management/account-management/src/lib.rsmodules/system/account-management/account-management/src/module.rsmodules/system/account-management/account-management/src/tr_plugin/error_map.rsmodules/system/account-management/account-management/src/tr_plugin/mod.rsmodules/system/account-management/account-management/src/tr_plugin/plugin_impl.rsmodules/system/account-management/account-management/src/tr_plugin/projection.rsmodules/system/account-management/account-management/src/tr_plugin/queries.rsmodules/system/account-management/account-management/src/tr_plugin/tests.rsmodules/system/account-management/docs/tr-plugin/DESIGN.mdmodules/system/account-management/docs/tr-plugin/PRD.mdmodules/system/account-management/docs/tr-plugin/features/feature-tenant-resolver-plugin.md
✅ Files skipped from review due to trivial changes (3)
- modules/system/account-management/account-management/src/lib.rs
- modules/system/account-management/account-management/Cargo.toml
- modules/system/account-management/docs/tr-plugin/DESIGN.md
🚧 Files skipped from review as they are similar to previous changes (7)
- modules/system/account-management/account-management/src/tr_plugin/mod.rs
- modules/system/account-management/account-management/src/config.rs
- modules/system/account-management/account-management/src/tr_plugin/plugin_impl.rs
- modules/system/account-management/account-management/src/tr_plugin/projection.rs
- modules/system/account-management/account-management/src/config_tests.rs
- modules/system/account-management/docs/tr-plugin/features/feature-tenant-resolver-plugin.md
- modules/system/account-management/account-management/src/tr_plugin/tests.rs
Implements `TenantResolverPluginClient` co-located inside AM, reading from AM-owned `tenants` and `tenant_closure` tables. Registers with `ClientHub` under a GTS-scoped instance id so the TR gateway discovers it via the same plugin pattern as `static-tr-plugin` / `rg-tr-plugin`. Covers all six SDK methods: `get_tenant`, `get_root_tenant`, `get_tenants`, `is_ancestor`, `get_ancestors`, `get_descendants`. Opt-in registration: `tr_plugin.enabled` defaults to `false` so a deploy that incidentally pulls AM into its binary does NOT register the plugin. When enabled, the plugin advertises in types-registry first (with idempotent `AlreadyExists` spec compare) and binds in `ClientHub` only after registry success — preserves the fail-closed init contract. `tr_plugin.vendor` and `priority` are config-driven; defaults intentionally lose selection to in-tree alternatives until the plugin is feature-complete. Hierarchy fail-close: `get_ancestors` and `get_descendants` compare hydrated `tenants` rows against the closure-row id set and surface `Internal` on mismatch (corruption defense). `get_descendants` additionally runs a depth-bounded post-walk completeness check so in-bound closure descendants unreachable via `parent_id` are flagged. `BarrierMode::Ignore` emits a structured audit warn under `target = "tr_plugin.audit"` after the visibility probe. Read-only role contract from DESIGN §3.5 is documented as a deferred follow-up (current implementation shares AM's writer pool pending `modkit-db` per-role pool abstraction); read-only behavior is enforced structurally in the module (no write calls anywhere) and pinned by a startup audit warning when `enabled = true`. 40+ SQLite-backed integration tests covering: provisioning invisibility, barrier semantics (Respect/Ignore for is_ancestor / get_ancestors / get_descendants), status-filter branch-preservation, TypesRegistry failure -> Internal, max_depth clamping, dedup, TenantNotFound edge cases, is_ancestor registry-independence, corruption fail-close (closure-tenants mismatch on both ancestor and descendant sides; in-bound unreachable under max_depth), config default pins (enabled=false, vendor matches resolver default, priority loses to in-tree alternatives), and validate() rejection of empty vendor.
diffora
force-pushed
the
am/06-tr-plugin-clean
branch
from
May 10, 2026 08:36
746d7f6 to
ee3d850
Compare
diffora
added a commit
that referenced
this pull request
May 13, 2026
* #15: partial-TX rollback on the apply seam's TOCTOU type guard. After ConversionService::approve runs its pre-apply TenantTypeChecker check, the converting tenant's tenant_type_uuid is mutated out of band. The apply TX MUST reject with Validation (expected_tenant_type_uuid mismatch) and leave every other piece of state intact: pending row stays Pending, tenants.self_managed unchanged, closure barriers unchanged. SQLite integration test. * #8 (partial): cross-tenant denial coverage for the conversion service's request_conversion and approve seams, plus the user service's provision_user. cancel + reject already had the fence; this commit closes the asymmetry on the remaining seams flagged by aviator5. Broader fan-out to list_own_for_tenant / list_inbound_for_parent and the user-side deprovision_user / list_users left as a follow-up issue — the load-bearing fence (require_caller_scope_or_not_found + scope-clamped tenant_repo lookup) is identical across these methods, so the pattern is pinned by the four tests landed here. * #20: provision_user happy path with the gts.cf.core.am.user.v1~ schema actually registered. The existing test silently exercised the schema-not-found short-circuit; the new test pins that the service-layer validator traverses the registered-schema path without short-circuiting. * #22: DoS surface tests on validate_new_user_payload_via_gts — 1 MB string value and 256-level nested object on the free-form `attributes` field. The current contract accepts both; the tests pin "no panic / no stack overflow / no hang" as the boundary. A future PR that adds an AM-side cap (max attributes byte size or recursion depth) flips the assertions to Validation without breaking the tests. Follow-up (not in this PR): - #14: module.rs::serve() ordering unit tests (4 fail-closed paths). Each path is currently exercised end-to-end through the integration harness; isolated unit tests on serve() require a sizeable mock harness around ModuleCtx that does not exist today. - #16: PG mirror of expire_tick + retention_soft_delete from SQLite integration. The SQLite suite covers the data-shape contract; the PG-only contract pins SERIALIZABLE behaviour that the existing pg_concurrent_approves_serialize_to_one_winner test already covers for the approve seam. - #8 broader: list_own / list_inbound / deprovision / list_users cross-tenant denial fan-out. All 456 unit tests + 11 SQLite integration tests + clippy-clean.
diffora
added a commit
that referenced
this pull request
May 13, 2026
Phase 1 dual-consent conversion lifecycle + IdP user-operations contract. Domain + storage land here; REST handlers in a follow-up. * Conversion state machine (pending → approved/cancelled/rejected/ expired) with partial-unique-pending invariant, dual-consent apply seam, type-stability TOCTOU guards, reaper + retention sweeps with `ConversionScope` + `CancellationToken`. * IdP user ops: `list_users` cursor pagination (MAX_TOP=200), `deprovision_user → Result<()>`, `IdpPluginSpecV1`. * IdP-metadata opaque-proxy contract: `tenant_idp_metadata` table, `TenantContext.metadata: Option<Value>`, mandatory `tenant_type`. * m0004 migration creates `conversion_requests` + `tenant_idp_metadata` with per-backend DDL; SQLite cascade backed by explicit DELETE in `hard_delete_one`. * Deep-review aviator5#4434462671: 20/22 findings closed; #4/#10/#14/#16 deferred to InTenantSubtree adoption (cyberfabric-core#1813). Signed-off-by: Diffora <ddiffora@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds the in-process Tenant Resolver plugin (
tr_plugin) co-located with AM — implements the fullTenantResolverPluginClientSDK surface against AM-ownedtenants+tenant_closuretables.get_tenant,get_root_tenant,get_tenants,is_ancestor,get_ancestors,get_descendantsTypesRegistryClientbatched reverse-hydration (tenant_type_uuid → tenant_type)status != 0predicate)BarrierMode::RespectaddsAND barrier = 0to closure queriesClientHubunder a GTS-scoped instance id31 integration tests on SQLite in-memory covering: happy-path reads, provisioning invisibility (all 6 SDK methods), barrier semantics (
Respect/Ignore), status filters,max_depthtraversal,TypesRegistryfailure →Internal, andis_ancestorregistry-independence.Test plan
cargo test -p cf-account-management— all 31tr_plugintests passBarrierMode::Respectclamps ancestors/descendants atbarrier=1edgesInternalon registry failureis_ancestorreturns correct bool even when registry always fails🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
Tests
Documentation