Conversation
eanzhao
left a comment
There was a problem hiding this comment.
Reviewed against CLAUDE.md architecture rules and the linked #364 minimum slice. Tests + build pass and the slice is correctly scoped to unblock #340, but the new write surface introduces ownership/auth/validation concerns that should at minimum be tracked as known follow-ups before this becomes the long-lived contract — see inline comments.
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## dev #427 +/- ##
==========================================
+ Coverage 70.58% 70.62% +0.03%
==========================================
Files 1176 1179 +3
Lines 84475 85165 +690
Branches 11127 11172 +45
==========================================
+ Hits 59630 60148 +518
- Misses 20539 20676 +137
- Partials 4306 4341 +35
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93b0c4f064
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
93b0c4f to
04e460e
Compare
eanzhao
left a comment
There was a problem hiding this comment.
All five comments addressed cleanly:
- Resource semantics gap —
// TODOadded in resolver pointing to actor-owned catalog; PUT binding now requires scope-admin role viaAevatarMemberAccessGuard.TryCreateScopeAdminRequiredResult. ✅ - memberId validation —
NormalizeMemberIdrejects:,/,\,?,#; new testResolveAsync_ShouldRejectMemberIdThatBreaksServiceKeySegmentscovers it. ✅ - Authorization — New
AevatarMemberAccessGuardenforces matching member claim or scope-admin role for reads/invoke, and scope-admin only for PUT binding; integration tests cover allow/deny paths viaX-Test-Member-Id/X-Test-Roleheaders. ✅ - Run lifecycle ops — Added
runs/{runId}/audit,:resume,:signal,:stopmember endpoints with full integration tests; doc claim softened and table updated. ✅ - AppId / Namespace leakage — Removed
AppIdfrom request DTO,AppId/Namespacefrom response DTO,appIdquery param from all member endpoints; newUpsertMemberScopeBindingHttpRequesthas clean public surface; doc adds explicit semantic. ✅
Resolver unit tests pass locally (3/3).
One minor observation, not blocking — feel free to fold into a follow-up: AevatarMemberAccessGuard.HasScopeAdminRole checks for any admin-typed role on the principal without validating it's bound to the requested scope. Today this is fine because AevatarScopeAccessGuard already enforces single-scope identity (rejects ambiguous scope claims), so admin-of-scope-A can't reach the member guard for scope-B. But if the system later supports multi-scope users, the role claim would need a scope binding (e.g., {type: "scope.role:scope-a", value: "admin"}) to avoid cross-scope admin escalation. Cheap to fix later when the actor-owned catalog lands.
dev added HandleInvokeMemberStreamAsync (PR #427 member-first APIs) which forwards to HandleInvokeStreamAsync. Pass IServiceRunRegistrationPort through so the new member-stream entry point also registers a service-run record before delegating, matching the other stream entry points. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Problem
Studio Bind / Invoke / Observe needs member-first backend routes so the frontend can address a member and let the backend resolve the published service id.
Closes #364
Solution
IMemberPublishedServiceResolverand a deterministic default resolver that maps normalizedmemberIdto the member-ownedpublishedServiceIdwhile rejecting service-key-unsafe member ids.appIdoverrides and app/namespace exposure from member resolver/response contracts.docs/2026-04-27-member-first-studio-apis.md.Impact Paths
src/Aevatar.Hostingsrc/platform/Aevatar.GAgentService.Abstractionssrc/platform/Aevatar.GAgentService.Applicationsrc/platform/Aevatar.GAgentService.Hostingtest/Aevatar.GAgentService.Teststest/Aevatar.GAgentService.Integration.Teststest/Aevatar.Studio.Testsdocs/Validation
dotnet test test/Aevatar.GAgentService.Tests/Aevatar.GAgentService.Tests.csproj --nologo --filter DefaultMemberPublishedServiceResolverTestsdotnet test test/Aevatar.Studio.Tests/Aevatar.Studio.Tests.csproj --nologo --filter ScopeEndpointAccessTestsdotnet test test/Aevatar.GAgentService.Integration.Tests/Aevatar.GAgentService.Integration.Tests.csproj --nologo --filter ScopeServiceEndpointsTestsdotnet test test/Aevatar.GAgentService.Tests/Aevatar.GAgentService.Tests.csproj --nologodotnet build aevatar.slnx --nologobash tools/ci/test_stability_guards.shbash tools/ci/query_projection_priming_guard.shgit diff --checkbash tools/ci/architecture_guards.shpasses through proto/channel/projection/workflow guards, then fails inplayground_asset_drift_guard.shbecause existing CLI playground build output differs fromdemos/Aevatar.Demos.Workflow.Web/wwwrootassets. This PR does not modify those frontend assets.