Multi-User/Agent Support for single AD4M nodes (1/3)#629
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| return AGENT.didForUser(this.#currentUserContext); | ||
| } | ||
| // Return cached main agent DID, or fallback to calling AGENT.did() | ||
| return this.#cachedMainAgentDid || AGENT.did(); |
There was a problem hiding this comment.
Synchronous getter returns Promise for user context DID
The did getter returns string but when #currentUserContext is set, it calls AGENT.didForUser() which according to agent_extension.d.ts returns Promise<string>. This causes the getter to return a Promise object instead of a string, breaking all code that uses agentService.did synchronously (e.g., string comparisons in LanguageController.ts at lines 291 and 340 will compare against [object Promise]).
Additional Locations (1)
| if (this.#currentUserContext) { | ||
| // @ts-ignore - New function from Rust | ||
| return AGENT.createSignedExpressionForUser(this.#currentUserContext, data); | ||
| } |
There was a problem hiding this comment.
Synchronous method returns Promise for user signed expressions
The createSignedExpression method returns Expression but when #currentUserContext is set, it calls AGENT.createSignedExpressionForUser() which returns Promise<Expression>. Callers like LanguageController.expressionCreate() at line 1012 use this synchronously with Literal.from(expr), which will receive a Promise object instead of an Expression, causing runtime errors when creating expressions in multi-user mode.
Additional Locations (1)
| if (this.#currentUserContext) { | ||
| // @ts-ignore - New function from Rust | ||
| return AGENT.agentForUser(this.#currentUserContext); | ||
| } |
There was a problem hiding this comment.
Synchronous getter returns Promise for user agent
The agent getter returns Agent but when #currentUserContext is set, it calls AGENT.agentForUser() which returns Promise<Agent>. This causes the getter to return a Promise object instead of an Agent, breaking any code that accesses agentService.agent properties synchronously in multi-user mode.
Additional Locations (1)
| getAllLocalUserDIDs(): string[] { | ||
| // @ts-ignore - Rust implementation includes main agent + managed users | ||
| return AGENT.getAllLocalUserDIDs(); | ||
| } |
There was a problem hiding this comment.
Synchronous method returns Promise for local user DIDs
The getAllLocalUserDIDs method returns string[] but calls AGENT.getAllLocalUserDIDs() which returns Promise<string[]>. This means all callers receive a Promise instead of an array. While linksAdapter.ts line 71 uses await, the method signature doesn't indicate it's async, causing type confusion and potential runtime issues when iterating over the "array".
Additional Locations (1)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Fix all issues with AI Agents 🤖
In @rust-executor/src/db.rs:
- Around line 2206-2233: The argon2 errors in hash_password and verify_password
are incorrectly wrapped as rusqlite::Error::ToSqlConversionFailure; update both
mapping closures to produce AnyError (e.g., use anyhow!("Failed to hash
password: {}", e) or e.into()) instead of the rusqlite wrapper so cryptographic
failures are represented as authentication/domain errors; replace the map_err
for Argon2::hash_password in hash_password and for PasswordHash::new in
verify_password accordingly.
In @rust-executor/src/graphql/graphql_types.rs:
- Around line 622-651: The UserInfo GraphQL type currently exposes last_seen as
Option<f64>, but the codebase uses the DateTime scalar (ISO-8601 string) for
timestamps; change UserInfo.last_seen from Option<f64> to the existing DateTime
scalar type (or Option<String> if DateTime is an alias) and update both From
impls (impl From<crate::types::User> for UserInfo and impl
From<crate::types::UserInfo> for UserInfo) to convert the source timestamp into
an ISO-8601 string/DateTime value (e.g., using chrono::Utc to format a
seconds/epoch value to rfc3339) instead of casting to f64 so GraphQL and JS
clients get consistent string-backed timestamps.
In @rust-executor/src/types.rs:
- Around line 523-543: The User struct currently derives Debug which will print
the sensitive password_hash; remove Debug from #[derive(...)] on the User
definition or replace it by implementing a custom impl std::fmt::Debug for User
that formats username, did, and last_seen but replaces password_hash with a
constant like "***redacted***"; ensure the UserInfo struct can keep Debug as-is
and update any uses that relied on the auto-derived Debug for User to use the
new manual Debug or alternative logging to avoid leaking the hash.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust-executor/src/graphql/subscription_resolvers.rs (1)
227-384: Link subscriptions ignore theuuidargument and filter only by owner DID.In
perspective_link_added/removed/updated:
- The
uuidargument is used only for an initialcan_access_perspectivecheck.- The actual subscription filter is derived solely from
get_agent_did_filter(owner DID).- The payload types (
PerspectiveLinkWithOwner,PerspectiveLinkUpdatedWithOwner) exposeperspective_uuid, but theirGetFilterimplementations return onlyowner.This means a subscription like
perspective_link_added(uuid: "P1")will receive link events for all perspectives owned by that user, and the GraphQL client cannot distinguish which perspective an event belongs to (since only the link is exposed viaGetValue). That’s a functional regression and can corrupt client state.Consider changing the filter to include both perspective and owner, e.g.:
- Make
GetFilterfor the wrapper types returnformat!("{}|{}", self.perspective_uuid, self.owner).- Compute the subscription filter similarly:
Some(format!("{}|{}", uuid, did)).This preserves multi-user isolation and the original per-perspective contract of these subscriptions.
🧹 Nitpick comments (3)
rust-executor/src/db.rs (3)
683-778: Owners persistence forperspective_handleis sound but JSON error handling could be clearer.Serializing
ownerswithunwrap_or_defaultand deserializing withand_then(|json| serde_json::from_str(&json).ok())gives a safe fallback, but it silently drops malformed JSON inowners.If you want stricter behavior, consider logging JSON parse failures (or propagating them) instead of returning
Noneso unexpected schema corruption is visible.
780-823:add_owner_to_neighbourhoodsilently no‑ops when neighbourhood is missing.When no row matches
shared_url,current_ownersbecomes[]and theUPDATEaffects 0 rows, but the function still returnsOk(()). That makes it hard to detect calls with an invalid/neighbourhood URL.Consider returning an error (or at least logging) when
UPDATE ... WHERE shared_url = ?2modifies 0 rows so callers can distinguish “owner added” from “no such neighbourhood”.
3291-3605: User/owners/last_seen tests provide good coverage; consider adding a negative case foradd_owner_to_neighbourhood.The new tests thoroughly exercise:
- User creation, duplicate handling, and password verification (including complex/unicode passwords).
- User table existence/schema.
- Neighbourhood owners addition and de‑duplication.
last_seentracking and user listing order.You might add a small test asserting behavior when calling
add_owner_to_neighbourhoodwith a non‑existentshared_url(currently a silent no‑op) to lock in the intended semantics.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
rust-executor/src/db.rsrust-executor/src/graphql/graphql_types.rsrust-executor/src/graphql/subscription_resolvers.rsrust-executor/src/types.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-24T14:26:44.307Z
Learnt from: lucksus
Repo: coasys/ad4m PR: 628
File: tests/js/tests/prolog-and-literals.test.ts:2726-2729
Timestamp: 2025-09-24T14:26:44.307Z
Learning: In AD4M, perspectives have names but are uniquely identified by UUIDs. Creating a new perspective with the same name in consecutive calls will create a fresh perspective each time, so there's no risk of state leakage between tests based on perspective names.
Applied to files:
rust-executor/src/db.rs
🧬 Code graph analysis (3)
rust-executor/src/graphql/subscription_resolvers.rs (3)
rust-executor/src/graphql/query_resolvers.rs (4)
agent(64-101)perspective(440-464)perspectives(552-599)can_access_perspective(20-60)rust-executor/src/agent/mod.rs (3)
did_for_context(103-119)did(143-145)new(256-271)rust-executor/src/agent/capabilities/mod.rs (1)
user_email_from_token(88-108)
rust-executor/src/graphql/graphql_types.rs (2)
rust-executor/src/db.rs (1)
new(64-277)rust-executor/src/graphql/query_resolvers.rs (1)
perspective(440-464)
rust-executor/src/db.rs (2)
rust-executor/src/types.rs (1)
to_string(460-464)rust-executor/src/graphql/graphql_types.rs (7)
default(1191-1193)default(1209-1211)new(528-543)new(1197-1205)new(1215-1229)get_owners(519-521)is_owned_by(498-503)
🪛 Gitleaks (8.30.0)
rust-executor/src/db.rs
[high] 3298-3298: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 3347-3347: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 3351-3351: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 3425-3425: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 3430-3430: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 3435-3435: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (15)
rust-executor/src/db.rs (3)
240-275: Schema additions and owners migration look consistent.
users/settingstables and theownerscolumn plusowner_did→ownersmigration are wired coherently and tolerate existing schemas via ignoredALTER TABLEerrors. This aligns with the new multi-user model without obvious migration hazards.
2310-2339: Settings and multi‑user toggle helpers are straightforward and cohesive.
get_setting/set_settingplusget_multi_user_enabled/set_multi_user_enabledare minimal and make the feature flag easy to query from other layers. Returningfalsewhen the setting is absent gives a predictable default.
2235-2301:NULLS LASTis supported in the bundled SQLite version—no change needed.SQLite has supported
ORDER BY ... NULLS LASTsince version 3.30.0 (2019-10-04). The project uses rusqlite 0.34.0, which bundles SQLite 3.49.1, so the syntax inlist_users()is valid and portable.The separation of concerns between
get_user_internalandget_user/list_usersremains a solid design choice for credential handling.rust-executor/src/graphql/graphql_types.rs (5)
473-566:PerspectiveHandle.ownersand helper methods match the new ownership model.The added
owners: Option<Vec<String>>plus helpers (add_owner,is_owned_by,is_unowned,get_primary_owner,get_owners,is_neighbourhood) give a clear, backwards‑compatible API:
- Unowned =
ownersmissing or empty.- Ownership checks are centralized and reused elsewhere (
can_access_perspective).- Constructors default
ownerstoNoneunless explicitly set.This structure looks consistent with the DB schema changes.
661-668:UserStatisticschoice ofDateTimeis consistent with the rest of the schema.Using the existing
DateTimescalar forlast_seenhere (vs. numeric inUserInfo) is fine as long as the resolver normalizes the internali64timestamp into a UTCDateTimeconsistently.
670-695: Neighbourhood signal and link owner wrappers align with new filtering.
NeighbourhoodSignalFilter’s composite filter"perspective_uuid|recipient_did"matches the new helper insubscription_resolvers, preserving backwards compatibility whenrecipientisNone.PerspectiveLinkWithOwnerandPerspectiveLinkUpdatedWithOwnerexposeownerfor filtering while keeping the GraphQL payload limited to link data viaGetValue.These types are the right building blocks; the main concern is how filters are computed on the subscription side (see comments in
subscription_resolvers.rs).
1010-1052: Owner-wrapped perspective subscription types are well-structured.
PerspectiveWithOwnerandPerspectiveRemovedWithOwnercleanly embed the owner DID into the payload while exposing only the handle/uuid viaGetValue, which is ideal for internal filtering and external API stability.
58-75: Theuser_didfield inAuthInfoInputis currently unused and not passed to authorization logic.While
user_didexists as an optional field in the GraphQL input, it is explicitly discarded during theFrom<AuthInfoInput> for AuthInfoconversion and never reaches any downstream authentication or authorization code. TheAuthInfostruct (which is used byrequest_capability()andpermit_capability()) has nouser_didfield at all.If this field was intended for future use, clarify its purpose. If it's not needed, consider removing it to avoid confusion.
Likely an incorrect or invalid review comment.
rust-executor/src/graphql/subscription_resolvers.rs (7)
25-71: DID filter helper correctly enforces user vs. main-agent behavior.
get_agent_did_filtercleanly separates:
- Main agent: optional DID filter (or None if not initialized).
- User agents: must have a resolvable DID or the subscription fails.
This is the right shape to avoid leaking data to user contexts without a valid DID.
73-107: Neighbourhood signal filter helper addresses prior leak concerns.
get_neighbourhood_signal_filternow:
- Uses
perspective_uuid|didfor user contexts and errors on DID resolution failure.- Falls back to perspective-only filtering only for the main agent (and only when no DID is available).
Combined with
NeighbourhoodSignalFilter::get_filter, this closes the earlier “fallback to perspective-only for users” issue.
170-202:neighbourhood_signalsubscription now safely enforces per-agent filters.Using
get_neighbourhood_signal_filterand aborting the subscription on error ensures:
- Users only see signals addressed to their DID for the specified perspective.
- The main agent retains backwards-compatible behavior.
The debug logging of perspective and filter is also helpful for diagnosing routing issues.
204-225:perspective_addednow respects per-owner isolation.Switching to
PerspectiveWithOwnerand filtering viaget_agent_did_filterensures subscribers only see perspective-add events for perspectives they own (or the main agent’s view), which matches the multi-user design.
386-411:perspective_removedis correctly owner-scoped via DID filter.Using
PerspectiveRemovedWithOwnerplusget_agent_did_filterensures only the owning agent (or main agent, as appropriate) receives remove events. Logging the filter aids debugging.
413-457:perspective_sync_state_changeenforces access but not per-owner filtering.You now:
- Check access via
can_access_perspectivebefore subscribing.- Filter only by
uuid(PerspectiveStateFilter’s filter is the perspective UUID).This means all authorized owners of a shared perspective see the same sync-state events, which is probably intended. If you ever need per-owner sync state, you’d need an owner-aware wrapper similar to the link subscriptions.
459-480:perspective_updatedfollows the same owner-based filtering asperspective_added.Using
PerspectiveWithOwnerwithget_agent_did_filteraligns updated events with the multi-user ownership model and avoids cross-user leaks.
| // Public UserInfo struct for GraphQL - only contains non-sensitive fields | ||
| // Note: last_seen uses f64 for GraphQL compatibility (GraphQL Int is limited to i32) | ||
| // f64 can safely represent integer timestamps up to 2^53 without precision loss | ||
| #[derive(GraphQLObject, Default, Debug, Deserialize, Serialize, Clone)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct UserInfo { | ||
| pub username: String, | ||
| pub did: String, | ||
| pub last_seen: Option<f64>, | ||
| } | ||
|
|
||
| impl From<crate::types::User> for UserInfo { | ||
| fn from(user: crate::types::User) -> Self { | ||
| UserInfo { | ||
| username: user.username, | ||
| did: user.did, | ||
| last_seen: user.last_seen.map(|ts| ts as f64), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl From<crate::types::UserInfo> for UserInfo { | ||
| fn from(user: crate::types::UserInfo) -> Self { | ||
| UserInfo { | ||
| username: user.username, | ||
| did: user.did, | ||
| last_seen: user.last_seen.map(|ts| ts as f64), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read the specified lines
head -n 660 rust-executor/src/graphql/graphql_types.rs | tail -n 60Repository: coasys/ad4m
Length of output: 1793
🏁 Script executed:
# Search for any custom scalar definitions or timestamp handling in the codebase
rg -i "scalar|timestamp|bigint" rust-executor/src/graphql/ -A 3Repository: coasys/ad4m
Length of output: 3481
🏁 Script executed:
# Check for juniper or coasys_juniper dependency and version
fd -e toml -e cargo | xargs grep -l "juniper\|coasys_juniper" 2>/dev/null | head -5Repository: coasys/ad4m
Length of output: 79
🌐 Web query:
GraphQL best practices representing 64-bit Unix timestamps juniper rust custom scalar
💡 Result:
Short guidance + options and a small Juniper pattern.
Key constraints
- GraphQL transport is JSON; JavaScript Numbers are only safe to 53 bits—so exposing raw 64-bit integers as JSON numbers can lose precision in many clients. [1][2].
- Juniper has built-in chrono/date types and supports custom scalars via #[derive(GraphQLScalar)] or #[graphql_scalar]. You can also implement ScalarValue to add u64/i64 storage. [3][4][5].
Recommended approaches (pick one)
- ISO 8601 DateTime string (recommended for interoperability)
- Represent instants as RFC3339/ISO-8601 strings (e.g. 2023-01-02T03:04:05Z). Safe, human-readable, well-supported by GraphQL clients and the-guild scalars. Good for most APIs. [6][1]
- Integer epoch as string (preserves full 64-bit value)
- Serialize the 64-bit unix timestamp (seconds or milliseconds) as a GraphQL String scalar. Clients parse into their preferred int type. Avoids JS-Number precision issues. Async-GraphQL documents this pattern and Juniper custom scalars can do the same. [4][2]
- Custom 64-bit scalar (if you want a typed scalar name like Timestamp)
- Implement a GraphQL custom scalar that serializes to/from a string (or to a JSON number only for non-JS clients). In Juniper you can:
- Newtype + #[derive(juniper::GraphQLScalar)] with #[graphql(transparent)] for simple delegation; or
- Use #[graphql_scalar] to implement custom parsing/serialization and validation; or
- Extend ScalarValue if you need server-side storage of i64/u64 in the GraphQL value system. See Juniper docs/examples. [3][5][8]
Practical Juniper pattern (serialize epoch as string)
- Use a newtype wrapper around i64/chrono::DateTime and implement GraphQL scalar that:
- to_output: outputs Value::String(self.0.to_string() or self.0.to_rfc3339())
- from_input: accept Value::String and parse to i64 or DateTime
This preserves 64-bit precision for JS clients and keeps typing in schema (scalar Timestamp). See Juniper scalars docs and async-graphql example of string-backed 64-bit scalar. [3][4]
Which unit: seconds vs milliseconds
- Pick and document one (commonly milliseconds in JS ecosystems, seconds in some systems). Always state the unit in the scalar description to avoid confusion. [6][2]
Summary recommendation
- Default to RFC3339 DateTime when you want human-readable instants and broad client convenience.
- Use a string-backed 64-bit scalar (scalar name Timestamp) if you must expose raw epoch values and retain full 64-bit precision for clients. Implement it in Juniper as a custom scalar/newtype per the docs. [6][3][4]
References
[1] Juniper Scalars docs (custom scalars & chrono support).
[2] GraphQL.js custom scalars guidance (when to use, serialization concerns; note JS Number precision).
[3] Juniper Scalars (book) — newtype and #[graphql_scalar] usage.
[4] Async-GraphQL example: string-backed 64-bit integer scalar.
[5] Juniper ScalarValue / implementing 64-bit storage.
If you want, I can produce a minimal Juniper #[graphql_scalar] code example for the string-backed 64-bit Timestamp (seconds or ms)—which unit do you prefer?
Consider aligning with the codebase's established timestamp pattern.
While f64 is safe for representing Unix timestamps in Rust, the codebase already uses ISO-8601 DateTime strings for other timestamp fields (e.g., PerspectiveExpression, LinkStatusUpdate). A custom GraphQL scalar wrapping a string—similar to the DateTime scalar already defined in this file—would be more consistent and better follows GraphQL best practices for 64-bit values. Using f64 doesn't actually prevent precision loss in JavaScript clients (both have a 2^53-bit safe integer limit); string-backed timestamps avoid this concern entirely.
🤖 Prompt for AI Agents
In @rust-executor/src/graphql/graphql_types.rs around lines 622-651, The
UserInfo GraphQL type currently exposes last_seen as Option<f64>, but the
codebase uses the DateTime scalar (ISO-8601 string) for timestamps; change
UserInfo.last_seen from Option<f64> to the existing DateTime scalar type (or
Option<String> if DateTime is an alias) and update both From impls (impl
From<crate::types::User> for UserInfo and impl From<crate::types::UserInfo> for
UserInfo) to convert the source timestamp into an ISO-8601 string/DateTime value
(e.g., using chrono::Utc to format a seconds/epoch value to rfc3339) instead of
casting to f64 so GraphQL and JS clients get consistent string-backed
timestamps.
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @rust-executor/src/db.rs:
- Around line 1704-1726: The export SELECT is missing the owners column and thus
exported objects lack owners; update the query string ".prepare(\"SELECT uuid,
name, neighbourhood, shared_url, state FROM perspective_handle\")" to include
owners, and modify the export routine that builds the JSON objects to add the
owners field (serialized the same way the import expects—an array/JSON string of
owner strings) so the import code that reads owners_json into perspective_handle
works correctly.
- Around line 780-822: The add_owner_to_neighbourhood currently treats a missing
perspective as an empty owners list and silently succeeds; change it to first
verify the perspective exists and return an error if not found: when fetching
owners in add_owner_to_neighbourhood (the SELECT on perspective_handle),
propagate the query error instead of using unwrap_or_default and map a
QueryReturnedNoRows into a clear Ad4mDbResult error (or explicitly check
existence with a COUNT/EXISTS query), then proceed to update; additionally,
after executing the UPDATE, check the affected row count and return an error if
zero so callers know the owner was not added.
♻️ Duplicate comments (1)
bootstrap-languages/p-diff-sync/index.ts (1)
50-74: Fix per-instance routing: validate recipient matches THIS instance's DID.The current validation checks if
recipientDidis any local user on the node (line 57), rather than checking if it matches this specific instance's agent DID. Each language instance is created for a specific user, so it should only accept signals addressed to that user.Impact: If the node hosts users A and B, the language instance for user A will incorrectly accept and process signals intended for user B, because
localUserDIDs.includes(recipientDid)returns true for both A and B.Fix: Use strict equality to ensure this instance only processes its own signals:
- if (payload.recipient_did) { - // Check if this signal is for THIS specific user (agent.did) - // Each language instance is created for a specific user - - const recipientDid = payload.recipient_did; - const localUserDIDs = await agent.getAllLocalUserDIDs(); - - if (! localUserDIDs.includes(recipientDid)) { - console.error(`[p-diff-sync] Received Signal not for user on this node. Recipient is ${payload.recipient_did}. All local user DIDs: ${localUserDIDs.join(', ')}`); - return; // Not for this user - } + if (payload.recipient_did) { + const recipientDid = payload.recipient_did as string; + + if (recipientDid !== agent.did) { + console.warn(`[p-diff-sync] Routed signal not for this language instance (have ${agent.did}, got ${recipientDid})`); + return; + }Note: This matches the guidance in the previous review comment for lines 49-74.
🧹 Nitpick comments (2)
executor/src/agent_extension.d.ts (1)
62-89: Consider adding JSDoc for consistency.The new multi-user methods are well-typed and consistent with the existing interface pattern. All past review concerns have been successfully addressed (synchronous return types, proper typing).
However,
didForUserhas comprehensive JSDoc documentation while the other new methods lack it. For consistency and improved developer experience, consider adding similar documentation to:
createSignedExpressionForUser(error conditions, what data types are accepted)agentForUser(behavior when user doesn't exist)listUserEmails(when it returns empty arrays)getAllLocalUserDIDs(when it returns empty arrays)📝 Example JSDoc pattern
/** * Creates a signed expression for a specific user. * * @param userEmail - The email address of the user * @param data - The data to include in the expression * @returns A signed Expression object * @throws {TypeError} If userEmail is not a non-empty string * @throws {Error} If no key is found for the given user email */ createSignedExpressionForUser: (userEmail: string, data: unknown) => Expression;bootstrap-languages/p-diff-sync/index.ts (1)
70-72: Avoid blocking: invoke callbacks concurrently.Awaiting callbacks serially means a slow callback blocks all subsequent callbacks from starting. Consider invoking them concurrently (e.g.,
Promise.allSettled) or as fire-and-forget to improve responsiveness.- for (const callback of telepresenceAdapter.signalCallbacks) { - await callback(perspectiveExpression, recipientDid); - } + await Promise.allSettled( + telepresenceAdapter.signalCallbacks.map(cb => cb(perspectiveExpression, recipientDid)) + );Apply the same pattern to the broadcast path (lines 77-79).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
bootstrap-languages/p-diff-sync/index.tsexecutor/src/agent_extension.d.tsrust-executor/src/db.rsrust-executor/src/types.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rust-executor/src/types.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-03T15:16:29.623Z
Learnt from: lucksus
Repo: coasys/ad4m PR: 619
File: rust-executor/src/perspectives/perspective_instance.rs:639-641
Timestamp: 2025-09-03T15:16:29.623Z
Learning: In the AD4M codebase, database operations like `add_many_links` during critical synchronization processes (such as `diff_from_link_language`) should use `.expect()` rather than error logging, as failures indicate severe system issues where fail-fast behavior is preferred over continuing with potentially corrupted state.
Applied to files:
rust-executor/src/db.rs
📚 Learning: 2025-09-24T14:26:44.307Z
Learnt from: lucksus
Repo: coasys/ad4m PR: 628
File: tests/js/tests/prolog-and-literals.test.ts:2726-2729
Timestamp: 2025-09-24T14:26:44.307Z
Learning: In AD4M, perspectives have names but are uniquely identified by UUIDs. Creating a new perspective with the same name in consecutive calls will create a fresh perspective each time, so there's no risk of state leakage between tests based on perspective names.
Applied to files:
rust-executor/src/db.rs
🧬 Code graph analysis (2)
bootstrap-languages/p-diff-sync/index.ts (7)
executor/src/core/agent/AgentService.ts (1)
agent(79-85)rust-executor/src/graphql/query_resolvers.rs (1)
agent(64-101)bootstrap-languages/p-diff-sync/linksAdapter.ts (1)
LinkAdapter(15-333)bootstrap-languages/centralized-p-diff-sync/linksAdapter.ts (1)
LinkAdapter(8-292)bootstrap-languages/centralized-p-diff-sync/telepresenceAdapter.ts (1)
TelepresenceAdapterImplementation(5-90)bootstrap-languages/p-diff-sync/telepresenceAdapter.ts (1)
TelepresenceAdapterImplementation(4-44)bootstrap-languages/p-diff-sync/happ.js (2)
BUNDLE(3-3)BUNDLE(3-3)
rust-executor/src/db.rs (1)
rust-executor/src/graphql/graphql_types.rs (4)
new(528-543)new(1197-1205)new(1215-1229)is_owned_by(498-503)
🪛 Gitleaks (8.30.0)
rust-executor/src/db.rs
[high] 3289-3289: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 3338-3338: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 3342-3342: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 3416-3416: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 3421-3421: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 3426-3426: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (5)
executor/src/agent_extension.d.ts (1)
1-1: LGTM! Import expanded to support multi-user methods.The addition of
Agentto the import is necessary for the newagentForUserreturn type and aligns with the multi-user support objectives.bootstrap-languages/p-diff-sync/index.ts (2)
17-18: LGTM! Observability improvements for multi-user debugging.The logging additions provide clear visibility into which agent context each language instance is operating under, which is valuable for troubleshooting multi-user signal routing.
Also applies to: 22-23, 83-83
26-37: LGTM! DNA registration correctly expanded for multi-user support.The additional zome calls (
render,commit,fast_forward_signal,get_others,add_active_agent_link) align with the multi-user feature requirements described in the PR objectives.rust-executor/src/db.rs (2)
2206-2224: LGTM! Password hashing implementation is secure.The password helpers correctly use Argon2 with random salts and proper error handling. The error mapping now uses
anyhow!instead of the incorrectrusqlite::Errorwrapper that was flagged in previous reviews. This is idiomatic Rust error handling for theAd4mDbResult<T> = Result<T, AnyError>type alias.
2227-2299: LGTM! User management properly separates internal and public APIs.The user management implementation addresses all security concerns from previous reviews:
- Password hash exposure eliminated: Public APIs (
get_user,list_users) returnUserInfowithoutpassword_hash- Internal function for verification:
get_user_internalis private and only used byverify_user_password- Timestamp overflow fixed: Uses
i64instead ofi32forlast_seen- Proper hashing: Passwords are hashed before storage in
add_userThe separation between
User(internal with password_hash) andUserInfo(public without password_hash) follows the principle of least privilege.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rust-executor/src/db.rs (1)
846-857: Consider distinguishing "not found" from database errors.The current implementation uses
unwrap_or_default()on the query result, which converts both "perspective not found" and real database errors (connection issues, corruption, etc.) into an empty vector. This can make debugging harder if an actual database problem occurs.Since the function returns
Ad4mDbResult, callers expect to be notified of errors. Consider either:
- Propagating all errors and letting callers handle "not found" explicitly, or
- Using
.optional()?to distinguishQueryReturnedNoRowsfrom other errors🔎 Proposed approach
pub fn get_neighbourhood_owners(&self, neighbourhood_url: &str) -> Ad4mDbResult<Vec<String>> { let owners: Vec<String> = self .conn .prepare("SELECT owners FROM perspective_handle WHERE shared_url = ?1")? .query_row([neighbourhood_url], |row| { let owners_json: String = row.get(0)?; - Ok(serde_json::from_str(&owners_json).unwrap_or_default()) + serde_json::from_str(&owners_json).map_err(|e| { + rusqlite::Error::FromSqlConversionFailure( + 0, + rusqlite::types::Type::Text, + Box::new(e), + ) + }) }) - .unwrap_or_default(); + .optional()? // Propagate errors, convert None to empty vec + .unwrap_or_default(); Ok(owners) }This way, real database errors are propagated while "not found" still returns an empty vec.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rust-executor/src/db.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-03T15:16:29.623Z
Learnt from: lucksus
Repo: coasys/ad4m PR: 619
File: rust-executor/src/perspectives/perspective_instance.rs:639-641
Timestamp: 2025-09-03T15:16:29.623Z
Learning: In the AD4M codebase, database operations like `add_many_links` during critical synchronization processes (such as `diff_from_link_language`) should use `.expect()` rather than error logging, as failures indicate severe system issues where fail-fast behavior is preferred over continuing with potentially corrupted state.
Applied to files:
rust-executor/src/db.rs
📚 Learning: 2025-09-24T14:26:44.307Z
Learnt from: lucksus
Repo: coasys/ad4m PR: 628
File: tests/js/tests/prolog-and-literals.test.ts:2726-2729
Timestamp: 2025-09-24T14:26:44.307Z
Learning: In AD4M, perspectives have names but are uniquely identified by UUIDs. Creating a new perspective with the same name in consecutive calls will create a fresh perspective each time, so there's no risk of state leakage between tests based on perspective names.
Applied to files:
rust-executor/src/db.rs
🧬 Code graph analysis (1)
rust-executor/src/db.rs (1)
rust-executor/src/graphql/graphql_types.rs (7)
new(528-543)new(1197-1205)new(1215-1229)default(1191-1193)default(1209-1211)get_owners(519-521)is_owned_by(498-503)
🪛 Gitleaks (8.30.0)
rust-executor/src/db.rs
[high] 3329-3329: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 3378-3378: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 3382-3382: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 3456-3456: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 3461-3461: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 3466-3466: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
2 participants
Summary
This PR implements comprehensive multi-user support across the AD4M stack, enabling multiple users to be hosted on a single executor instance with proper isolation, authentication, and routing.
This is the first of (likely) 3 PRs. Email sending for user registration and app-level capability token requests will follow quickly after in a 2nd, and CAL compliant key handling is pushed out to the 3nd change set. The intention is to use this as an MVP of this approach to gather performance data of ad4m running many users to assess feasibility of this federated approach.
Motivation
Enable AD4M to function as a multi-tenant platform where a single executor can securely host multiple users, each with their own identity, perspectives, and neighbourhoods, while maintaining proper isolation and access control.
-> Users are not required to run their AD4M node on their own hardware.
-> Community admins or app developers can easily offer to host AD4M instances for users.
-> Maintaining ability for users to run completely local and p2p and seamlessly switch over
Changes
🔐 User Management & Authentication
runtimeCreateUser,runtimeLoginUser)subfielduserstable)👥 Agent Context System
AgentContextto trackuser_emailandis_main_agentstatusdid_for_context(),sign_for_context(),create_signed_expression()with context🏘️ Perspective & Neighbourhood Ownership
owner_didtoownersarray in perspectivesadd_owner_to_neighbourhood,get_neighbourhood_owners)📡 Signal Routing
RoutedSignalPayloadstructure withrecipient_didfieldget_agents_did_keys()🔗 Language Updates
p-diff-syncandp-diff-sync-socket-signalingfor multi-user signal routingLinksAdapternow creates DID links for all local users during syncgetAllLocalUserDIDs()for proper signal delivery🌐 Ad4mConnect
connectMultiUser()method for multi-user connection modemultiUser,backendUrl,userEmail,userPasswordTesting
Checklist
🔮 Future Work
follow-up PR
Migration Notes
owner_didfield in perspectives has been migrated to anownersarrayFuture Considerations
💡 CAL Compliance
The current implementation uses locally-generated keys for managed users. A follow-up PR will address CAL (Cryptographic Autonomy License) compliant key derivation and revocation mechanisms to ensure proper security standards and user key sovereignty for hosted/managed users.
Related Issues
Screenshots/Demos
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Note
Enables hosting multiple users on a single executor with isolation and targeted routing.
p-diff-synczome now takesmy_didincommit/sync; addsRoutedSignalPayloadand recipient-aware signal handling; DID↔pubkey links per user;get_others/DID queries updated for multi-user.runtimeCreateUser,runtimeLoginUser,runtimeMultiUserEnabled/Set,runtimeListUsers; addsUserCreationResult; perspectives now exposeowners; telepresence callbacks carry optionalrecipientDid.didForUser, per-user signing, list local DIDs); JWTsubset to user email; newruntime.user_managementcapabilities; throttled last-seen tracking.connectMultiUser), remote URL detection, login/signup UI, connection overview; websocket subprotocol fix.--enable_multi_userand TLS--tls_port; integration test jobintegration-tests-multi-user-simple; shared CI setup command; minor .gitignore updates.Written by Cursor Bugbot for commit 6a6032f. This will update automatically on new commits. Configure here.