Memory Optimizations #644
Memory Optimizations #644
Conversation
📝 WalkthroughWalkthroughThis PR shifts property/collection access and mutations to metadata-driven SurrealDB flows (with Prolog-first attempts), introduces Prolog execution modes and per-context pooling, adds shared Whisper model caching, expands SDNA-aware helpers, hardens SurrealQL escaping, and skips/adjusts several Prolog-dependent tests. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant PerspectiveProxy
participant Ad4mModel
participant PrologService
participant SurrealDB
Client->>PerspectiveProxy: request data
PerspectiveProxy->>Ad4mModel: getData()
alt Prolog-first
Ad4mModel->>PrologService: run Prolog query
PrologService-->>Ad4mModel: results or error/empty
alt Prolog success
Ad4mModel->>Ad4mModel: apply metadata transforms
Ad4mModel-->>PerspectiveProxy: resolved data
else
Ad4mModel->>SurrealDB: query using metadata-driven SurrealQL
SurrealDB-->>Ad4mModel: links/values
Ad4mModel->>Ad4mModel: map/format values (language/author/timestamp)
Ad4mModel-->>PerspectiveProxy: data
end
else SdnaOnly/no-Prolog
Ad4mModel->>SurrealDB: direct metadata-driven query
SurrealDB-->>Ad4mModel: values
Ad4mModel-->>PerspectiveProxy: data
end
PerspectiveProxy-->>Client: response
sequenceDiagram
actor Client
participant PerspectiveInstance
participant PrologService
participant SimpleEngine
participant PrologEnginePool
Client->>PerspectiveInstance: run query
alt Simple / SdnaOnly mode
PerspectiveInstance->>PrologService: run_query_simple()
PrologService->>SimpleEngine: ensure engine updated if dirty
SimpleEngine->>SimpleEngine: execute query
SimpleEngine-->>PrologService: results
else Pooled mode
PerspectiveInstance->>PrologService: _run_query()
PrologService->>PrologEnginePool: ensure/get pool for context
PrologEnginePool->>PrologEnginePool: execute query
PrologEnginePool-->>PrologService: results
else Disabled
PrologService-->>PerspectiveInstance: return empty / no-op
end
PerspectiveInstance-->>Client: respond
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rust-executor/src/perspectives/perspective_instance.rs (2)
2297-2318: Simple-mode prolog update flow likely breaks notifications/surreal subs + can double-publish diffs.
Inspawn_prolog_facts_update()Simple mode:
- it sets only
trigger_prolog_subscription_check, but does not settrigger_notification_check/trigger_surreal_subscription_check(so those loops may never run after link changes).- it calls
pubsub_publish_diff(diff)from the spawned task while many callers also callpubsub_publish_diff(...)afterward → duplicate events.- publishing from the spawned task can race with
update_surreal_cache()in the caller, so subscribers may observe events before Surreal is updated.Suggested: don’t publish diffs in
spawn_prolog_facts_update()(leave publishing to callers after Surreal update), and set all relevant triggers in Simple mode.Minimal fix sketch (remove pubsub publish, set all triggers)
- if PROLOG_MODE == PrologMode::Simple { + if PROLOG_MODE == PrologMode::Simple { log::debug!("Prolog facts update (Simple mode): marking subscription engine dirty"); // Trigger subscription check to rerun all subscriptions with updated data *(self_clone.trigger_prolog_subscription_check.lock().await) = true; - - self_clone.pubsub_publish_diff(diff).await; + // Keep behavior consistent with pooled mode: notifications + surreal subscriptions must re-check too + *(self_clone.trigger_notification_check.lock().await) = true; + *(self_clone.trigger_surreal_subscription_check.lock().await) = true; if let Some(sender) = completion_sender { let _ = sender.send(()); } return; }Also applies to: 2430-2437
3770-3805: Periodic logging may leak query contents / be noisy at info-level.
Consider lowering todebug!, sampling fewer IDs, or guarding behind a feature flag / env var in production.
🤖 Fix all issues with AI agents
In `@core/src/model/Ad4mModel.ts`:
- Around line 771-778: The query currently interpolates this.#baseExpression
directly into linksQuery which risks SQL injection; change the call to use a
parameterized SurrealQL query or safely escape the value instead of string
concatenation: modify how linksQuery is built and the invocation of
this.#perspective.querySurrealDB (referencing linksQuery and
this.#baseExpression) so the base expression is passed as a bound parameter (or
run through a proper SurrealDB escaping utility) rather than injected into the
SQL string.
In `@core/src/perspectives/PerspectiveProxy.ts`:
- Around line 1406-1448: getCollectionValuesViaSurreal currently performs N+1
isSubjectInstance() calls per value and emits production console logs; change it
to batch-check instances via a single/limited-number SurrealDB query (use
querySurrealDB to fetch instance membership for all values at once or in chunks)
or run isSubjectInstance in parallel with a concurrency cap, remove or replace
console.log debug statements with guarded/non-sensitive logging, and mark
getCollectionValuesViaSurreal as private to match the property fallback;
reference the getCollectionValuesViaSurreal method, collMeta.instanceFilter,
isSubjectInstance, and querySurrealDB when implementing these changes.
- Around line 1365-1404: The getPropertyValueViaSurreal method should decode
literal targets and be made non-public: mark getPropertyValueViaSurreal as
private (or protected) and, when propMeta.resolveLanguage === 'literal' or the
retrieved value is a literal URL (value.startsWith('literal://')), return
Literal.fromUrl(value).get() instead of the raw URL; otherwise keep the existing
expression resolution flow (calling getExpression and JSON.parse fallback) and
the original fallbacks, ensuring Literal is imported/available before use.
- Around line 1508-1525: getAllSubjectProxies currently returns Prolog-like
binding objects ({ X: base }) which violates its declared Promise<T[]> return
type and JSDoc; replace the binding mapping with construction of Subject proxies
like getAllSubjectInstances does (e.g., for each result use new Subject(this,
result.base, className) and collect those), update the method return type/JSDoc
to match Subject instances (or, if you intend to return bindings, rename the
method to getAllSubjectBindings and update its signature and docs accordingly)
and remove the console mapping that creates { X: base } so the method returns
actual Subject proxies consistent with getAllSubjectInstances.
- Around line 1084-1131: The method currently spams console.log and builds
SurrealQL by interpolating unescaped values (expression, triple.predicate,
triple.target) which can break queries; remove the console.log lines and escape
single quotes in all interpolated values before building queries (or switch to
parameterized queries if supported by querySurrealDB), e.g., add a small helper
escapeSurreal(value: string) that replaces ' with \\' and use it when composing
checkQuery for both the zero-requiredTriples branch and the requiredTriples loop
(references: metadata.requiredTriples, querySurrealDB, expression,
triple.predicate, triple.target); also revisit the early return false when
metadata is missing to match other methods (either return a safe default or add
the optional Prolog fallback infer path such as invoking
infer(`subject_class("${className}", C), instance(C, "${expression}")`) if
classes may exist without SDNA).
- Around line 1153-1329: In getSubjectClassMetadataFromSDNA: replace the literal
URL construction `literal://string:${className}` with the canonical form using
Literal.from(className).toUrl(); add a helper escapeRegExp(str) and apply it to
interpolated names used in dynamic RegExp creation (the property_getter regex
that uses propName and the collection_getter regex that uses collName) to avoid
unescaped special chars and ReDoS risks; and make the instance rule extraction
more robust by using a DOTALL/global-safe pattern (e.g., match the full
instance(...) :- ... rule body with [\s\S]*? and/or iterate all instance rule
matches) instead of the fragile /instance\([^,]+,\s*\w+\)\s*:-\s*(.+?)\./s so
requiredTriples extraction does not miss clauses. Ensure you update references
in the code paths that build regexes (the getterMatch/new RegExp for
property_getter, getterLinePattern for collection_getter) and keep using the
same requiredTriples/requiredPredicates flow.
In `@rust-executor/src/perspectives/perspective_instance.rs`:
- Around line 1819-1852: When handling PrologMode::SdnaOnly in
perspective_instance.rs, avoid materializing all links by filtering to SDNA
links before mapping into DecoratedLinkExpression; call
self.get_links_local(&LinkQuery::default()).await? then run the existing
extract_sdna_links on that result (or filter by link.sdna flag) and only map
those into DecoratedLinkExpression before passing to service.run_query_simple
(and any other call sites identified: the blocks around ensure_engine_updated
and run_query_simple usage). Update the branches for PrologMode::SdnaOnly where
links are collected (functions/getters: get_links_local, extract_sdna_links,
DecoratedLinkExpression::from, run_query_simple) so only SDNA links are
allocated and passed through.
In `@rust-executor/src/prolog_service/mod.rs`:
- Around line 28-51: The hard-coded PROLOG_MODE = PrologMode::SdnaOnly prevents
link-data queries in production; replace the static with a lazily-initialized
value that reads the AD4M_PROLOG_MODE environment variable (defaulting to
"Simple") using lazy_static (or once_cell) and parse the string into the
PrologMode enum (implement FromStr or match on the string values "Simple",
"Pooled", "SdnaOnly", "Disabled"); update references to use the new
lazy-initialized PROLOG_MODE and ensure tests can override the env var without
recompilation.
- Around line 104-233: ensure_engine_updated currently holds the write lock on
self.simple_engines across expensive awaits (PrologEngine::spawn,
PoolUtils::preprocess_program_lines, load_module_string) and also stores full
current_links even in SdnaOnly mode; to fix, minimize lock scope by: acquire the
write lock only to check/insert a placeholder SimpleEngine entry (create a
minimal SimpleEngine placeholder or clone the entry you need, e.g., clone
Arc/RwLock handles or move PrologEngine instances out), then release the lock
before calling PrologEngine::spawn, PoolUtils::preprocess_program_lines, and
load_module_string; once the heavy async work is done, reacquire the write lock
to update the engines map and set fields. Also, when PROLOG_MODE ==
PrologMode::SdnaOnly, avoid storing full current_links on SimpleEngine (clear or
leave as empty) and only set current_sdna_links via
Self::extract_sdna_links(links) for change detection.
In `@tests/js/tests/prolog-and-literals.test.ts`:
- Around line 434-455: The setup that picks or clears a todo title can crash if
Todo.all() returns an empty array; update the test to first assert or guard that
todos.length > 0 and if empty create a dedicated test Todo (use the Todo.create
or factory method) so you never index todos[0]; alternatively, explicitly throw
a clear, descriptive error if no todos are available. Locate the logic around
Todo.all(perspective!), the loop checking t.title, and the branch that uses
todos[0] and replace it with a safe guard that either constructs a fresh Todo or
asserts presence before attempting to read todos[0] and manipulate links via
perspective!.get(new LinkQuery(...)) and todo.baseExpression.
🧹 Nitpick comments (5)
tests/js/tests/social-dna-flow.ts (1)
9-10: Test skip is appropriately documented.The comment clearly explains why the test is skipped in SdnaOnly mode. Consider creating a tracking issue to re-enable this test when/if full Prolog mode support is restored or alternative SDNA flow testing is implemented.
Would you like me to open an issue to track re-enabling this test in the future?
tests/js/tests/prolog-and-literals.test.ts (1)
1846-1858: Polling loops: avoid noisy console logs + consider a sharedwaitForhelper
The loops are fine, but repeatedconsole.login test runs gets loud and slows CI. A small helper (timeout + interval + predicate) would reduce duplication and noise.Also applies to: 2573-2578
core/src/model/Ad4mModel.ts (1)
811-819: Unnecessary.datacheck onLiteral.get()result.According to the
Literalclass implementation,get()returns the literal value directly (string, number, boolean, or parsed JSON object), not an object with a.dataproperty. The.datacheck is dead code sinceparsed.datawill typically beundefined.♻️ Simplified fix
} else if (typeof value === 'string' && value.startsWith('literal://')) { // Parse literal URL try { - const parsed = Literal.fromUrl(value).get(); - value = parsed.data !== undefined ? parsed.data : parsed; + value = Literal.fromUrl(value).get(); } catch (e) { // Keep original value } }rust-executor/src/perspectives/perspective_instance.rs (1)
795-806: Repeated “mark dirty” blocks: consider centralizing + avoid extra locks.
This pattern is duplicated across multiple mutation paths and re-lockspersistedand callsget_prolog_service()each time. A small helper (e.g.,mark_prolog_dirty_if_simple(&self)) would reduce repetition and avoid incidental ordering drift.Also applies to: 911-922, 1045-1056, 1233-1244, 1348-1359, 4042-4054
rust-executor/src/prolog_service/mod.rs (1)
776-777: Test ignore is OK short-term, but consider mode-parameterizing tests.
IfPROLOG_MODEbecomes runtime-configurable, you can run this test underPooledin CI instead of ignoring it globally.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
core/src/model/Ad4mModel.tscore/src/model/Subject.tscore/src/model/decorators.tscore/src/perspectives/PerspectiveProxy.tsrust-executor/src/ai_service/mod.rsrust-executor/src/graphql/mutation_resolvers.rsrust-executor/src/perspectives/perspective_instance.rsrust-executor/src/prolog_service/engine_pool.rsrust-executor/src/prolog_service/mod.rstests/js/tests/perspective.tstests/js/tests/prolog-and-literals.test.tstests/js/tests/runtime.tstests/js/tests/social-dna-flow.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-17T16:34:09.725Z
Learnt from: jhweir
Repo: coasys/ad4m PR: 617
File: ad4m-hooks/vue/src/useModel.ts:45-56
Timestamp: 2025-07-17T16:34:09.725Z
Learning: In ad4m-hooks/vue/src/useModel.ts, the baseExpression property is defined as a getter on the Ad4mModel prototype, not as an own property on individual instances. The current implementation correctly uses Object.defineProperty to create enumerable own properties that shadow the prototype getter, which is the appropriate approach for making prototype accessors enumerable.
Applied to files:
core/src/model/Subject.tscore/src/model/Ad4mModel.ts
📚 Learning: 2025-04-11T14:17:11.180Z
Learnt from: lucksus
Repo: coasys/ad4m PR: 593
File: rust-executor/src/prolog_service/engine_pool.rs:65-89
Timestamp: 2025-04-11T14:17:11.180Z
Learning: The `run_query_all()` method in PrologEnginePool intentionally uses a write lock (not a read lock) because it can run assert/1 queries that modify facts. This ensures all engines remain synchronized and prevents other operations from executing concurrently during fact updates.
Applied to files:
rust-executor/src/perspectives/perspective_instance.rsrust-executor/src/prolog_service/mod.rs
📚 Learning: 2025-09-25T14:08:01.975Z
Learnt from: lucksus
Repo: coasys/ad4m PR: 628
File: core/src/model/Ad4mModel.ts:1078-1092
Timestamp: 2025-09-25T14:08:01.975Z
Learning: Collection method names in Ad4mModel are pluralized (e.g., addIngredients, removeIngredients, setCollectionIngredients) as generated by the Collection decorator using `add${capitalize(propertyName)}`. The util functions like collectionToAdderName that convert to singular are used for different purposes, not for the primary method naming convention.
Applied to files:
core/src/model/Ad4mModel.ts
🧬 Code graph analysis (5)
rust-executor/src/graphql/mutation_resolvers.rs (1)
rust-executor/src/agent/capabilities/mod.rs (1)
check_capability(37-77)
rust-executor/src/ai_service/mod.rs (1)
rust-executor/src/pubsub.rs (3)
new(23-27)get_global_pubsub(117-119)publish(37-42)
tests/js/tests/prolog-and-literals.test.ts (3)
core/src/model/Ad4mModel.ts (1)
perspective(613-615)rust-executor/src/graphql/mutation_resolvers.rs (2)
perspective(387-391)links(1772-1775)core/src/perspectives/PerspectiveResolver.ts (1)
perspective(66-68)
rust-executor/src/perspectives/perspective_instance.rs (1)
rust-executor/src/prolog_service/mod.rs (1)
get_prolog_service(764-767)
core/src/model/Ad4mModel.ts (2)
core/src/model/decorators.ts (2)
PropertyOptions(147-194)CollectionOptions(404-419)core/src/Literal.ts (1)
Literal(9-84)
🪛 ast-grep (0.40.5)
core/src/perspectives/PerspectiveProxy.ts
[warning] 1241-1241: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(property_getter\\([^,]+,\\s*[^,]+,\\s*"${propName}"[^)]*\\)\\s*:-\\s*triple\\([^,]+,\\s*"([^"]+)")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 1282-1282: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(collection_getter\\([^,]+,\\s*[^,]+,\\s*"${collName}"[^)]*\\)\\s*:-[^.]+\\.)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (31)
rust-executor/src/graphql/mutation_resolvers.rs (1)
2443-2453: Verify the capability change fromAI_CREATE_CAPABILITYtoAI_PROMPT_CAPABILITY.The
ai_add_taskmutation now requiresAI_PROMPT_CAPABILITYinstead ofAI_CREATE_CAPABILITY. This seems inconsistent with the CRUD pattern used elsewhere:
ai_remove_taskusesAI_DELETE_CAPABILITYai_update_taskusesAI_UPDATE_CAPABILITYPlease confirm this is intentional. If users with prompt capability should also be able to create tasks, consider documenting this design decision.
core/src/model/decorators.ts (1)
100-114: Dual-path resolution approach looks correct.The Prolog-first approach with graceful fallback to SdnaOnly is well-structured. The try-catch ensures errors in Prolog inference don't break the query flow.
Consider adding a debug log in the catch block (line 112-114) to help diagnose cases where Prolog fails silently, which could be useful during development.
core/src/model/Subject.ts (2)
53-88: Property getter dual-path logic is well-implemented.The Prolog-first approach with appropriate null/undefined/empty checks and graceful fallback to SurrealDB is correctly implemented. The nested try-catch for expression resolution (lines 59-72) handles edge cases where the expression URI might fail to resolve.
122-141: Collection getter implementation looks good.The filtering of empty strings (line 126) before returning Prolog results is a good defensive measure. The fallback to SurrealDB with appropriate logging maintains consistency with the property getter pattern.
rust-executor/src/ai_service/mod.rs (3)
36-37: Timeout and interval reductions are reasonable.Reducing
TRANSCRIPTION_TIMEOUT_SECSfrom 120s to 30s andTRANSCRIPTION_CHECK_INTERVAL_SECSfrom 10s to 5s will help clean up idle streams faster, reducing memory pressure.
1117-1152: Shared Whisper model caching architecture is well-designed.The approach of keying by model size and storing
Arc<Whisper>in a shared HashMap is correct. The lock scope is appropriately limited, and the logging provides good visibility into model reuse.One minor note: consider adding a method to clear/evict unused models from the cache if memory pressure becomes an issue in the future.
1166-1168: This concern is unfounded. The code is correct.Kalosm's
Whispertype internally usesArcfor model weights, makingclone()cheap. Thetranscribe()method requires ownership (takes the value by move), so the clone is necessary. The code comment on lines 1166-1167 explicitly states "The model weights stay shared in memory!" — model weights are not duplicated. This is the intended design for sharing models across multiple transcription streams.Likely an incorrect or invalid review comment.
tests/js/tests/runtime.ts (1)
238-239: LGTM - Test appropriately skipped for SdnaOnly mode.The skip is correctly applied since the notification trigger relies on Prolog
triple()queries (line 252), which won't function in SdnaOnly mode where links aren't loaded into the Prolog engine.Consider creating a tracking issue to ensure this test gets re-enabled once notifications are migrated to work with SurrealDB queries, so it doesn't remain skipped indefinitely.
tests/js/tests/perspective.ts (3)
312-313: LGTM - Test correctly skipped for SdnaOnly mode.This test relies on Prolog
triple()queries with link data (line 317), which is unavailable in SdnaOnly mode. The explanatory comment is helpful.
359-360: LGTM - Prolog query test appropriately disabled.This test directly exercises Prolog
triple()andlink()queries (lines 372, 377, 379) which require link data in the Prolog engine. Skipping is the correct approach for SdnaOnly mode.
846-847: LGTM - Subscription test correctly skipped.This test uses
subscribeInfer()with Prolog queries (line 859), which depends on link data being loaded into the Prolog engine. The skip is appropriate for SdnaOnly mode.rust-executor/src/prolog_service/engine_pool.rs (2)
42-42: Helpful documentation addition.The inline comment clarifying how to disable filtered pools (
set to usize::MAX) improves code maintainability.
45-46: Acknowledge reference counting mitigation but verify impact on intermittent non-subscription access.The pool timeout reduction (15 min → 2 min) is significant, but reference counting partially mitigates the stated concern. Pools with active subscriptions or held references are preserved indefinitely, regardless of the 2-minute timeout. However, the timeout still affects intermittent access patterns where users release references and return later (e.g., 3–5 minutes), triggering unnecessary pool recreation.
Pool recreation costs remain valid:
- Memory allocation for new engines
- Population from complete pool data
- Potential latency spike for first query after recreation
The code lacks production metrics to validate whether this tradeoff is acceptable. Consider either:
- Monitoring actual pool recreation frequency post-deployment to justify the aggressive timeout
- Adopting the suggested middle ground (5 minutes / 300 seconds) to reduce churn while still saving memory
- Documenting this as a deliberate memory optimization with understood latency tradeoff
tests/js/tests/prolog-and-literals.test.ts (3)
531-537: Cleanup looks good; consider scoping the cleanup to the current test’s created instances if possible
This removes allad4m://type -> ad4m://messagelinks in the shared perspective, which is fine for isolation, but can hide unintended cross-test interactions. If you can track created message roots per test, cleanup becomes more surgical.
924-928: Good change: avoid asserting collection ordering when backend query ordering isn’t guaranteed
This makes the test resilient to ordering differences introduced by the SurrealDB path.
421-432: Fix typo in comment at line 2831: "moded" → "mode"The comment "skipped because only applies to prolog-pooled moded" should read "mode".
Regarding conditional skips: The 6 unconditional
it.skip()anddescribe.skip()statements (lines 421, 488, 509, 778, 2335, 2832) have comments suggesting they're skipped due to SdnaOnly limitations. However, this test file runs with fixed configuration (hardcoded "prolog-agent" appDataPath) and no environment flag is currently available to conditionally gate these tests. To implement conditional skips, the test infrastructure would need to expose a configuration flag that this test file can check before running.core/src/perspectives/PerspectiveProxy.ts (1)
1331-1363: The schema is consistent with this query pattern. Thenodetable and->linkedge relationship are properly defined in the SurrealDB schema (rust-executor/src/surreal_service/mod.rs:258–266), and theFROM node WHERE count(->link[WHERE ...])pattern used here is extensively tested and validated in the Ad4mModel.test.ts test suite. This query approach is standard throughout the codebase for instance discovery and will work as intended with the deployed schema.Likely an incorrect or invalid review comment.
core/src/model/Ad4mModel.ts (9)
617-624: LGTM!Clean implementation of metadata accessor that reads from the prototype's decorator-populated
__propertiesstructure.
626-633: LGTM!Consistent pattern with
getPropertyMetadatafor accessing collection metadata.
635-660: LGTM!Good defensive error handling for custom setters and missing predicates. The action structure is correct for the
executeActionAPI.
662-690: LGTM!Clear action type mapping and consistent error handling pattern with
generatePropertySetterAction.
1487-1506: LGTM!Good defensive check for empty string targets and improved error logging for debugging expression resolution failures.
1984-2003: LGTM!Clean metadata-driven implementation replacing the Prolog query. Good fallback behavior with warning on missing metadata.
2005-2028: LGTM!Consistent metadata-driven pattern for collection setter with proper array/single-value handling.
2030-2076: LGTM!Consistent implementation for adder and remover with appropriate parallel execution for array values.
2176-2194: LGTM!Good refactor to explicitly handle arrays as collections and skip collection properties when setting regular properties. This prevents metadata confusion between the two types.
rust-executor/src/perspectives/perspective_instance.rs (2)
54-56: Subscription timeout drop to 60s may evict healthy clients.
If clients send keepalive >60s (or are backgrounded), subscriptions will silently time out; consider making this configurable or aligning with frontend keepalive cadence.
3613-3684: Good: avoids cloninglast_resultfor each subscription check.
This should reduce per-interval allocations significantly for large subscription payloads.Also applies to: 3691-3730
rust-executor/src/prolog_service/mod.rs (3)
246-333: Simple/SdnaOnly query + subscription split (separate engines) looks solid.
Using a dedicated subscription engine avoids interference with “regular” queries and matches the intent described in the PR.
339-360: Mode-gating pooled APIs is clear and consistent.
The early-return behavior prevents accidental pooled usage in Simple/SdnaOnly/Disabled modes and improves debuggability via logs.Also applies to: 422-453, 462-486, 519-543, 614-636, 691-714
52-72: [rewritten comment]
[classification tag]
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
… + remove production logs
There was a problem hiding this comment.
Actionable comments posted: 3
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/perspectives/perspective_instance.rs (1)
3984-4017: Privacy/logging: avoid logging query contents atinfo(may include sensitive data).The periodic log prints subscription IDs plus a query preview at
info. Queries can embed user data (or identifiers), andinfocan end up in production logs.Suggested change: log only counts at
info, and move query previews todebug(or guard behind a feature flag).
🤖 Fix all issues with AI agents
In `@core/src/model/Ad4mModel.ts`:
- Around line 814-822: The literal parsing branch incorrectly expects
Literal.fromUrl(value).get() to return an object with a data property; instead
assign the returned parsed value directly to value. Locate the block using
Literal.fromUrl(value).get() (inside the string 'literal://' branch) and replace
the conditional that checks parsed.data !== undefined with a direct assignment
value = parsed (while retaining the try/catch to fall back to the original value
on error).
In `@rust-executor/src/perspectives/perspective_instance.rs`:
- Around line 1382-1420: get_sdna_links_local() claims to perform two targeted
queries but relies on get_links_local(), whose predicate branch currently calls
db.get_all_links(&uuid) and filters in-memory, negating the targeted behavior;
either add an indexed DB accessor such as Ad4mDb::get_links_by_predicate(&uuid,
predicate) and update get_links_local() (or call that DB method directly from
get_sdna_links_local()) so predicate queries hit the index, or if you cannot add
a DB API now, change the get_sdna_links_local() doc comment to stop claiming
“two targeted queries” and clearly state it may scan all links; update
references to get_links_local, get_sdna_links_local, and consider adding
Ad4mDb::get_links_by_predicate to align callers and tests.
- Around line 2518-2532: The Simple-mode branch in spawn_prolog_facts_update is
causing duplicate pubsub events because it calls
self_clone.pubsub_publish_diff(diff).await before returning; remove that publish
call from the PrologMode::Simple early-return path (keep marking
trigger_prolog_subscription_check and sending completion_sender if present) so
publishing is performed only by the caller and not duplicated inside the
tokio::spawn block.
♻️ Duplicate comments (1)
core/src/perspectives/PerspectiveProxy.ts (1)
1414-1431: Add literal URL decoding for property values.When
resolveLanguageis not set but the retrieved value is aliteral://URL, the method returns the raw URL string instead of the decoded value. This was noted in a previous review.Proposed fix
const value = result[0].value; + // Decode literal URLs + if (typeof value === "string" && value.startsWith("literal://")) { + try { + return Literal.fromUrl(value).get(); + } catch (e) { + // Fall through to existing handling + } + } + // Handle expression resolution if needed if (propMeta.resolveLanguage && value) {
🧹 Nitpick comments (5)
core/src/perspectives/PerspectiveProxy.ts (1)
1524-1535: Minor: Unused variablefirstSet.The variable
firstSeton line 1529 is declared but not used. The filtering logic correctly usesvalidExpressionSets.every()instead.Proposed cleanup
// Find intersection: expressions that passed ALL required triple checks if (validExpressionSets.length === 0) { return expressions; } - const firstSet = validExpressionSets[0]; const validExpressions = expressions.filter(expr => { return validExpressionSets.every(set => set.has(expr)); });core/src/model/Ad4mModel.ts (1)
1366-1384: Consider usingescapeSurrealStringinternally to reduce duplication.
formatSurrealValueduplicates the escaping logic fromescapeSurrealString. Consider refactoring to use the imported utility internally for consistency and reduced maintenance burden.Proposed refactor
private static formatSurrealValue(value: any): string { if (typeof value === 'string') { - // Escape backslashes first, then single quotes and other special characters - const escaped = value - .replace(/\\/g, '\\\\') // Backslash -> \\ - .replace(/'/g, "\\'") // Single quote -> \' - .replace(/"/g, '\\"') // Double quote -> \" - .replace(/\n/g, '\\n') // Newline -> \n - .replace(/\r/g, '\\r') // Carriage return -> \r - .replace(/\t/g, '\\t'); // Tab -> \t - return `'${escaped}'`; + return `'${escapeSurrealString(value)}'`; } else if (typeof value === 'number' || typeof value === 'boolean') {rust-executor/src/prolog_service/mod.rs (1)
225-235: Consider usingOptionortakepattern instead of placeholder engines.The
std::mem::replacewithPrologEngine::new()placeholders works but creates unspawned engine instances that are immediately discarded. This is minor overhead.💡 Alternative using Option pattern
Wrapping the engines in
Option<PrologEngine>inSimpleEnginewould allow usingtake()without creating placeholders:// In SimpleEngine struct: query_engine: Option<PrologEngine>, subscription_engine: Option<PrologEngine>, // Then use: let query_engine_to_update = simple_engine.query_engine.take().unwrap(); let subscription_engine_to_update = simple_engine.subscription_engine.take().unwrap(); // And restore: simple_engine.query_engine = Some(query_engine_to_update); simple_engine.subscription_engine = Some(subscription_engine_to_update);rust-executor/src/perspectives/perspective_instance.rs (2)
795-805: Reduce duplication: extract a helper for “mark query engine dirty in Simple mode”.The same
if PROLOG_MODE == PrologMode::Simple { ... mark_dirty(...) }block repeats across multiple mutation paths; it also re-lockspersistedeach time.Possible refactor sketch
+ async fn mark_simple_query_engine_dirty(&self) { + if PROLOG_MODE != PrologMode::Simple { + return; + } + let perspective_uuid = self.persisted.lock().await.uuid.clone(); + get_prolog_service().await.mark_dirty(&perspective_uuid).await; + }Then replace the repeated blocks with:
- if PROLOG_MODE == PrologMode::Simple { - let perspective_uuid = self.persisted.lock().await.uuid.clone(); - get_prolog_service().await.mark_dirty(&perspective_uuid).await; - } + self.mark_simple_query_engine_dirty().await;Also applies to: 911-921, 1045-1055, 1233-1244, 1348-1359, 4256-4267
3827-3886: Good memory win (avoid cloninglast_result), butjoin_allcan still stampede.Avoiding
last_resultclones is a solid improvement, butfuture::join_all(query_futures).awaitwill fire all subscription re-queries concurrently; with many subscriptions, that can spike CPU/memory and hammer Prolog/SurrealDB.Consider bounding concurrency (e.g.,
stream::iter(...).buffer_unordered(N)or a semaphore) to cap simultaneous queries.Also applies to: 3905-3950
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/src/model/Ad4mModel.tscore/src/perspectives/PerspectiveProxy.tscore/src/utils.tsrust-executor/src/perspectives/perspective_instance.rsrust-executor/src/perspectives/sdna.rsrust-executor/src/prolog_service/mod.rstests/js/tests/prolog-and-literals.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/js/tests/prolog-and-literals.test.ts
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: lucksus
Repo: coasys/ad4m PR: 644
File: core/src/perspectives/PerspectiveProxy.ts:1465-1491
Timestamp: 2026-01-15T14:55:56.365Z
Learning: The codebase is moving toward making Prolog completely optional. Do not suggest adding Prolog fallbacks or dependencies in code paths that are being migrated to SurrealDB-based implementations (SdnaOnly mode).
📚 Learning: 2026-01-15T14:55:56.365Z
Learnt from: lucksus
Repo: coasys/ad4m PR: 644
File: core/src/perspectives/PerspectiveProxy.ts:1465-1491
Timestamp: 2026-01-15T14:55:56.365Z
Learning: The codebase is moving toward making Prolog completely optional. Do not suggest adding Prolog fallbacks or dependencies in code paths that are being migrated to SurrealDB-based implementations (SdnaOnly mode).
Applied to files:
rust-executor/src/perspectives/perspective_instance.rsrust-executor/src/prolog_service/mod.rs
📚 Learning: 2025-04-11T14:17:11.180Z
Learnt from: lucksus
Repo: coasys/ad4m PR: 593
File: rust-executor/src/prolog_service/engine_pool.rs:65-89
Timestamp: 2025-04-11T14:17:11.180Z
Learning: The `run_query_all()` method in PrologEnginePool intentionally uses a write lock (not a read lock) because it can run assert/1 queries that modify facts. This ensures all engines remain synchronized and prevents other operations from executing concurrently during fact updates.
Applied to files:
rust-executor/src/perspectives/perspective_instance.rsrust-executor/src/prolog_service/mod.rs
📚 Learning: 2025-07-17T16:34:09.725Z
Learnt from: jhweir
Repo: coasys/ad4m PR: 617
File: ad4m-hooks/vue/src/useModel.ts:45-56
Timestamp: 2025-07-17T16:34:09.725Z
Learning: In ad4m-hooks/vue/src/useModel.ts, the baseExpression property is defined as a getter on the Ad4mModel prototype, not as an own property on individual instances. The current implementation correctly uses Object.defineProperty to create enumerable own properties that shadow the prototype getter, which is the appropriate approach for making prototype accessors enumerable.
Applied to files:
core/src/model/Ad4mModel.ts
📚 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/prolog_service/mod.rs
📚 Learning: 2026-01-15T14:55:56.365Z
Learnt from: lucksus
Repo: coasys/ad4m PR: 644
File: core/src/perspectives/PerspectiveProxy.ts:1465-1491
Timestamp: 2026-01-15T14:55:56.365Z
Learning: In code paths that are migrated to SurrealDB-based implementations (SdnaOnly mode), do not introduce Prolog fallbacks or add Prolog dependencies. Apply this guidance to TypeScript files under core/src/perspectives (and related SdnaOnly areas) to ensure Prolog remains optional only where appropriate and is not wired into SurrealDB-driven flows.
Applied to files:
core/src/perspectives/PerspectiveProxy.ts
🧬 Code graph analysis (2)
rust-executor/src/perspectives/perspective_instance.rs (4)
rust-executor/src/prolog_service/mod.rs (2)
get_prolog_service(802-805)new(76-81)rust-executor/src/prolog_service/engine_pool.rs (1)
new(118-138)rust-executor/src/prolog_service/pool_trait.rs (1)
new(74-82)rust-executor/src/prolog_service/filtered_pool.rs (1)
new(311-322)
core/src/perspectives/PerspectiveProxy.ts (2)
core/src/utils.ts (1)
escapeSurrealString(49-57)core/src/Literal.ts (1)
Literal(9-84)
🪛 ast-grep (0.40.5)
core/src/perspectives/PerspectiveProxy.ts
[warning] 1270-1270: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(property_getter\\([^,]+,\\s*[^,]+,\\s*"${escapedPropName}"[^)]*\\)\\s*:-\\s*triple\\([^,]+,\\s*"([^"]+)")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 1313-1313: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(collection_getter\\([^,]+,\\s*[^,]+,\\s*"${escapedCollName}"[^)]*\\)\\s*:-[^.]+\\.)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (23)
core/src/utils.ts (1)
49-57: LGTM! Well-implemented SurrealQL escaping utility.The function correctly handles the most common special characters that could break queries or enable injection attacks. The order of replacements (backslash first) is correct to prevent double-escaping issues.
Minor consideration: For maximum robustness, you might also consider escaping null bytes (
\0) which can cause issues in some database engines, though this is rarely encountered in practice.core/src/perspectives/PerspectiveProxy.ts (5)
416-427: LGTM! Standard regex escaping implementation.This correctly escapes all regex metacharacters to prevent ReDoS attacks and regex injection when building dynamic patterns for SDNA parsing.
1098-1153: LGTM! SQL injection concerns addressed with proper escaping.The implementation now correctly uses
escapeSurrealStringfor all interpolated values in SurrealQL queries (lines 1116, 1126-1131). The count value handling properly accounts for different response formats from SurrealDB.
1175-1244: LGTM! Regex injection concerns addressed with escapeRegExp.The implementation now properly escapes
propNameandcollNamebefore using them in dynamic RegExp patterns (lines 1270, 1313). The static analysis warnings are effectively false positives since the escaped strings cannot contain regex metacharacters.Note: The method makes multiple
infer()calls per property/collection for metadata extraction. For models with many properties, consider caching the metadata after first extraction or batching the Prolog queries if this becomes a performance bottleneck.
1433-1476: LGTM! N+1 query issue addressed with batch checking.The implementation now uses
batchCheckSubjectInstances(line 1468) to efficiently batch-check instance membership instead of individualisSubjectInstancecalls per element. The fallback tofilterInstancesSequentialwhen metadata is unavailable is a reasonable safety net.
1561-1632: LGTM! SurrealDB-based instance retrieval.Both
getAllSubjectInstancesandgetAllSubjectProxiesnow correctly use SurrealDB queries via metadata when available. The intentional omission of a Prolog fallback aligns with the goal of making Prolog completely optional. Based on learnings, this is the intended direction.The
getAllSubjectProxiesmethod now correctly returns Subject proxy instances (addressing a past review concern about returning binding objects).core/src/model/Ad4mModel.ts (5)
618-634: LGTM! Clean metadata accessor helpers.These methods provide a clean interface to access decorator-stored metadata, supporting the Phase 1 migration away from Prolog queries.
636-691: LGTM! Metadata-driven action generation.The action generators correctly replace Prolog queries with metadata-derived actions. The explicit error for custom setters (line 644-647) is a reasonable Phase 1 limitation with a clear error message guiding users.
1991-2083: LGTM! Clean migration to metadata-driven mutations.The property and collection setters now use decorator metadata instead of Prolog queries. The graceful handling of missing metadata with
console.warn(instead of throwing) provides good developer experience during migration. The parallel execution pattern withPromise.allfor collection operations is appropriate.
2183-2201: LGTM! Improved array/collection handling in innerUpdate.The explicit handling of arrays as collections (lines 2183-2188) and the check to skip collection-annotated keys during property updates (lines 2192-2196) correctly prevents the dual-treatment issue. Empty arrays being skipped is reasonable behavior.
1494-1537: LGTM! Robust target value handling.The guard on line 1494 correctly prevents processing of empty, null, or undefined targets. The warning logs for failed expression resolution (lines 1511-1513) are appropriate for debugging. The literal URL fallback (lines 1515-1527) handles edge cases gracefully.
rust-executor/src/perspectives/sdna.rs (1)
113-117: LGTM!The helper function is well-documented and correctly combines the two SDNA-related checks. Using
as_deref()for the Option comparison is idiomatic.rust-executor/src/prolog_service/mod.rs (10)
28-51: LGTM - acknowledged as intentional per prior discussion.The
PrologModeenum and hardcodedSdnaOnlydefault align with the memory optimization goals. The enum design is clean and well-documented.
53-73: LGTM!The
SimpleEnginestruct design with separate query and subscription engines is sound. UsingOption<Vec<...>>forcurrent_sdna_linkscorrectly distinguishes uninitialized state from empty SDNA links.
83-101: LGTM!Mode-gated dirty marking with appropriate lock scope. The method correctly no-ops for Disabled/Pooled modes.
284-326: LGTM!The method correctly gates on Prolog mode, ensures the engine is updated, normalizes the query, and executes through the query engine. Error handling is appropriate.
328-370: LGTM!Properly uses the separate
subscription_engineto avoid interference with regular queries. Structure mirrorsrun_query_simpleappropriately.
372-423: LGTM!Mode gating correctly restricts pool creation to Pooled mode. The optimistic locking with double-check pattern is the right approach for avoiding race conditions.
455-614: LGTM!Consistent mode-gating across all pooled-mode query methods. The error handling for misuse (calling pooled methods in Simple/SdnaOnly mode) with warning logs is helpful for debugging.
644-712: LGTM!Graceful no-op behavior in non-pooled modes with
tracelevel logging (appropriate for non-error conditions). Performance timing instrumentation is useful for debugging.
716-785: LGTM!Mode gating correctly routes Simple/SdnaOnly modes to lazy updates. The trace-level logging for expected no-op behavior is appropriate.
814-920: LGTM!The test is correctly marked as ignored since it exercises Pooled mode functionality which is incompatible with the current
SdnaOnlydefault. The test implementation is valid for when Pooled mode is enabled.rust-executor/src/perspectives/perspective_instance.rs (1)
54-56: Verify client compatibility with the new 60s subscription timeout.Dropping
QUERY_SUBSCRIPTION_TIMEOUTto 60s is a behavioral change; if clients don’t send keepalives reliably (or mobile backgrounding delays them), subscriptions may churn.Please confirm expected keepalive cadence on the client side (and in any proxies) is comfortably < 60s under real-world conditions.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| .await | ||
| .mark_dirty(&perspective_uuid) | ||
| .await; | ||
| } |
There was a problem hiding this comment.
See if we can simplify the simple mode handling and merge it with the rest
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/js/tests/prolog-and-literals.test.ts (1)
421-432: Add a tracking note for this skipped test.
Skipping reduces coverage forInstanceQuery(condition: ..). Please add a TODO/issue link or rationale so it’s clear when to re-enable.
🤖 Fix all issues with AI agents
In `@core/src/model/Ad4mModel.ts`:
- Around line 769-772: The code currently selects the first matching link with
matching[0], which picks the oldest link because links are ordered ASC; change
this to pick the latest link by using the last element (e.g.,
matching[matching.length - 1]) or reverse/order DESC before selecting. Update
the selection in the block that filters links by propMeta.predicate (variables:
links, matching, propMeta.predicate, link) so the "latest wins" semantics are
enforced.
- Around line 2163-2176: innerUpdate currently skips empty arrays so calling
setCollectionSetter(key, value, batchId) only for non-empty arrays prevents
clearing collections by assigning []; change innerUpdate so Array.isArray(value)
always calls setCollectionSetter (remove the value.length check) and ensure
setCollectionSetter handles an empty array by removing/clearing existing
collection links for the given key (so setting [] clears the collection). Update
references in innerUpdate, setCollectionSetter, and any logic that relies on
getCollectionMetadata to treat arrays as collections consistently.
- Around line 641-660: In generatePropertySetterAction, add a guard that checks
metadata.writable (or metadata.readOnly flag if used) and reject setter
generation for read-only properties: if metadata.writable === false then throw a
clear Error like `Property "${key}" is read-only and cannot be written` (place
this check before returning the action and before any custom setter handling so
`@ReadOnly` fields are protected); keep the existing checks for metadata.setter
and metadata.through and preserve the existing action shape when writable is
allowed.
- Around line 1993-2013: The current truthy checks around setter/adder/remover
logic (using if (value)) in Ad4mModel.ts drop valid falsy values like 0, false,
and ""; update the conditional to a nullish check (value !== null && value !==
undefined or value != null) wherever you call generateCollectionAction and then
call this.#perspective.executeAction (referenced symbols: getCollectionMetadata,
generateCollectionAction, this.#perspective.executeAction, this.#baseExpression)
so that legitimate falsy values are passed through; apply the same change to the
other occurrences noted (lines around 2018-2037 and 2042-2061).
- Around line 752-761: The SurrealDB query in getData() (linksQuery) is missing
a perspective constraint, allowing links from other perspectives; update the
WHERE clause in the linksQuery (constructed with safeBaseExpression via
ctor.formatSurrealValue) to also filter by the current perspective
(this.#perspective) — use the same approach you use elsewhere (either bind a
$perspective parameter or escape this.#perspective with formatSurrealValue) so
the query only returns rows where perspective equals the current perspective
before calling this.#perspective.querySurrealDB.
In `@rust-executor/src/db.rs`:
- Around line 1176-1211: get_links_by_predicate currently returns
LinkExpression.data.predicate as Some("") when the DB stored an empty sentinel,
causing inconsistency with add_link/get_link which map "" → None; update
get_links_by_predicate to normalize the predicate value by reading the raw
string from row.get(2) and converting an empty string to None (i.e., set
Link.data.predicate = None when the retrieved predicate == ""), and consider
factoring this normalization into a shared helper used by add_link, get_link,
and get_links_by_predicate for consistency.
In `@rust-executor/src/perspectives/perspective_instance.rs`:
- Around line 1800-1820: The doc for mark_prolog_engine_dirty says it applies to
"Simple/SdnaOnly" but the implementation only checks PrologMode::Simple; either
update the runtime check to include SdnaOnly (e.g., check PROLOG_MODE ==
PrologMode::Simple || PROLOG_MODE == PrologMode::SdnaOnly) or change the doc
comment to remove SdnaOnly to match current behavior; adjust the check in the
mark_prolog_engine_dirty method (and ensure any callers like
update_prolog_engines keep expected semantics) and reference the PrologMode enum
and PROLOG_MODE global to locate where to change.
In `@rust-executor/src/prolog_service/mod.rs`:
- Around line 167-307: The map is left with unspawned placeholder engines if
load_module_string() fails after swapping engines out; wrap the expensive load
operations for query_engine_to_update and subscription_engine_to_update in
error-handling that on any Err reacquires self.simple_engines.write().await and
restores the original engines into the SimpleEngine (assign
simple_engine.query_engine = query_engine_to_update and
simple_engine.subscription_engine = subscription_engine_to_update), set
simple_engine.dirty = true (so update will be retried), then return the error;
use the existing symbols (simple_engines, SimpleEngine, query_engine_to_update,
subscription_engine_to_update, load_module_string) and avoid leaving
PrologEngine::new() placeholders in the map on failure.
♻️ Duplicate comments (5)
tests/js/tests/prolog-and-literals.test.ts (5)
495-514: Duplicate: skipped-test coverage needs tracking.
Same concern as Line 421.
516-529: Duplicate: skipped-test coverage needs tracking.
Same concern as Line 421.
785-807: Duplicate: skipped-test coverage needs tracking.
Same concern as Line 421.
2342-2395: Duplicate: skipped-test coverage needs tracking.
Same concern as Line 421.
2838-2840: Duplicate: skipped-test coverage needs tracking.
Same concern as Line 421.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/src/model/Ad4mModel.tscore/src/model/Subject.tsrust-executor/src/db.rsrust-executor/src/perspectives/perspective_instance.rsrust-executor/src/prolog_service/mod.rstests/js/tests/prolog-and-literals.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/model/Subject.ts
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: lucksus
Repo: coasys/ad4m PR: 644
File: core/src/perspectives/PerspectiveProxy.ts:1465-1491
Timestamp: 2026-01-15T14:55:56.365Z
Learning: The codebase is moving toward making Prolog completely optional. Do not suggest adding Prolog fallbacks or dependencies in code paths that are being migrated to SurrealDB-based implementations (SdnaOnly mode).
Learnt from: lucksus
Repo: coasys/ad4m PR: 644
File: rust-executor/src/perspectives/perspective_instance.rs:2518-2532
Timestamp: 2026-01-15T17:26:10.403Z
Learning: In rust-executor/src/perspectives/perspective_instance.rs, the Simple-mode branch of spawn_prolog_facts_update() intentionally publishes diffs so both Prolog subscribers and link-only clients receive updates. Do not remove this publish or “dedupe” it by default; duplicate add/remove events are an accepted trade-off unless explicitly changed later.
📚 Learning: 2026-01-15T17:29:55.923Z
Learnt from: lucksus
Repo: coasys/ad4m PR: 644
File: core/src/model/Ad4mModel.ts:814-822
Timestamp: 2026-01-15T17:29:55.923Z
Learning: In core/src/model/Ad4mModel.ts, when parsing literal URLs with Literal.fromUrl(value).get() in the getData() method's SurrealDB fallback path (around lines 814-822), the code correctly checks for a `data` property (`parsed.data !== undefined ? parsed.data : parsed`) to handle two cases: Expression objects (which have a data property that should be extracted when we want just the value) and other literal types (which should be used as-is). This conditional is intentional and correct *in this specific context* where we want to reduce Expressions to their data values. Note that this is not a universal pattern—Expression shapes are preserved with full metadata in other contexts where that's needed.
Applied to files:
tests/js/tests/prolog-and-literals.test.ts
📚 Learning: 2026-01-15T14:55:56.365Z
Learnt from: lucksus
Repo: coasys/ad4m PR: 644
File: core/src/perspectives/PerspectiveProxy.ts:1465-1491
Timestamp: 2026-01-15T14:55:56.365Z
Learning: The codebase is moving toward making Prolog completely optional. Do not suggest adding Prolog fallbacks or dependencies in code paths that are being migrated to SurrealDB-based implementations (SdnaOnly mode).
Applied to files:
tests/js/tests/prolog-and-literals.test.tsrust-executor/src/perspectives/perspective_instance.rscore/src/model/Ad4mModel.tsrust-executor/src/prolog_service/mod.rs
📚 Learning: 2026-01-15T17:26:10.403Z
Learnt from: lucksus
Repo: coasys/ad4m PR: 644
File: rust-executor/src/perspectives/perspective_instance.rs:2518-2532
Timestamp: 2026-01-15T17:26:10.403Z
Learning: In rust-executor/src/perspectives/perspective_instance.rs, the Simple-mode branch of spawn_prolog_facts_update() intentionally publishes diffs so both Prolog subscribers and link-only clients receive updates. Do not remove this publish or “dedupe” it by default; duplicate add/remove events are an accepted trade-off unless explicitly changed later.
Applied to files:
rust-executor/src/perspectives/perspective_instance.rsrust-executor/src/prolog_service/mod.rs
📚 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/perspectives/perspective_instance.rsrust-executor/src/prolog_service/mod.rs
📚 Learning: 2025-04-11T14:17:11.180Z
Learnt from: lucksus
Repo: coasys/ad4m PR: 593
File: rust-executor/src/prolog_service/engine_pool.rs:65-89
Timestamp: 2025-04-11T14:17:11.180Z
Learning: The `run_query_all()` method in PrologEnginePool intentionally uses a write lock (not a read lock) because it can run assert/1 queries that modify facts. This ensures all engines remain synchronized and prevents other operations from executing concurrently during fact updates.
Applied to files:
rust-executor/src/perspectives/perspective_instance.rsrust-executor/src/prolog_service/mod.rs
📚 Learning: 2025-07-17T16:34:09.725Z
Learnt from: jhweir
Repo: coasys/ad4m PR: 617
File: ad4m-hooks/vue/src/useModel.ts:45-56
Timestamp: 2025-07-17T16:34:09.725Z
Learning: In ad4m-hooks/vue/src/useModel.ts, the baseExpression property is defined as a getter on the Ad4mModel prototype, not as an own property on individual instances. The current implementation correctly uses Object.defineProperty to create enumerable own properties that shadow the prototype getter, which is the appropriate approach for making prototype accessors enumerable.
Applied to files:
core/src/model/Ad4mModel.ts
📚 Learning: 2026-01-15T17:29:55.923Z
Learnt from: lucksus
Repo: coasys/ad4m PR: 644
File: core/src/model/Ad4mModel.ts:814-822
Timestamp: 2026-01-15T17:29:55.923Z
Learning: In core/src/model/Ad4mModel.ts, within the getData() method's SurrealDB fallback path (around lines 814-822), retain the conditional: when parsing literal URLs with Literal.fromUrl(value).get(), use parsed.data !== undefined ? parsed.data : parsed. This is intentional and correct for reducing Expression objects to their data values in this specific context, while preserving full metadata for other contexts where Expression shapes are needed. Do not generalize this pattern to all literal parsing; this behavior is scoped to this file and scenario.
Applied to files:
core/src/model/Ad4mModel.ts
🧬 Code graph analysis (2)
core/src/model/Ad4mModel.ts (2)
core/src/model/decorators.ts (2)
PropertyOptions(147-194)CollectionOptions(404-419)core/src/utils.ts (1)
escapeSurrealString(49-57)
rust-executor/src/prolog_service/mod.rs (4)
rust-executor/src/prolog_service/engine_pool.rs (1)
new(118-138)rust-executor/src/prolog_service/sdna_pool.rs (1)
new(231-238)rust-executor/src/prolog_service/pool_trait.rs (2)
new(74-82)preprocess_program_lines(137-147)rust-executor/src/perspectives/sdna.rs (4)
get_data_facts(656-669)get_sdna_facts(672-749)get_static_infrastructure_facts(284-653)is_sdna_link(103-111)
🔇 Additional comments (28)
tests/js/tests/prolog-and-literals.test.ts (5)
436-462: Good guard + cleanup for deterministic selection.
This avoids accidental cross-test contamination and gives a clear failure mode when prerequisites aren’t met.
538-544: Nice isolation cleanup.
Removing Message flags inafterEachshould prevent test bleed.
929-935: Order-insensitive assertion is a good stability tweak.
This avoids flaky ordering assumptions while still validating contents.
1930-1972: Polling + relaxed counts look reasonable for async subscriptions.
The looped waits andat.leastchecks make the test more resilient.
2580-2585: Polling safeguard improves subscription reliability.
The retry loop helps stabilize the async assertions.core/src/model/Ad4mModel.ts (5)
6-6: No issues in these helper additions.Also applies to: 618-634, 668-691
814-818: No issues here.
920-945: Predicate escaping hardening looks good.Also applies to: 1063-1063, 1195-1195, 1271-1273, 1309-1322
1474-1492: No issues here.
1972-1990: Metadata-driven property setter path looks consistent.rust-executor/src/db.rs (1)
3081-3102: Test looks good. Covers predicate-based filtering with two predicates and validates results independently.rust-executor/src/perspectives/perspective_instance.rs (7)
23-23: Config tweaks look fine.
ImportingPrologMode/PROLOG_MODEand tightening the subscription timeout are straightforward.Also applies to: 54-54
795-797: Consolidated Prolog update helper usage is consistent.
Good to seeupdate_prolog_engines(...)applied across the main mutation paths.Also applies to: 902-904, 1027-1030, 1207-1209, 1313-1315, 4109-4112
1338-1376: SDNA link retrieval is now properly targeted.
The two-query SDNA fetch plus indexed predicate lookup should reduce unnecessary scans.Also applies to: 1391-1392
1822-1889: Mode-aware query routing looks solid.
The Simple/SdnaOnly vs Pooled/Disabled branching is clear and consistent across the query entry points.Also applies to: 1899-1928, 1938-1959, 1970-1997, 2010-2087, 2100-2204
3679-3721: Subscription checks now avoid cloning large results.
Nice memory optimization by deferringlast_resultcloning until after change detection.Also applies to: 3757-3795
3836-3869: Periodic subscription logging is helpful for observability.
The interval-based summary should make stuck subscriptions easier to diagnose.
2371-2384: SdnaOnly still pushes non‑SDNA data into Prolog.
spawn_prolog_facts_update()only special-casesSimple. InSdnaOnly, it proceeds into assertion/full rebuild logic, which can assert all link facts and rebuild fromget_links()—undermining the SdnaOnly memory goal. Add an SdnaOnly branch to ignore non‑SDNA diffs and rebuild using SDNA-only links.Based on learnings, avoid reintroducing Prolog data facts in SdnaOnly.🛠️ Suggested direction
if PROLOG_MODE == PrologMode::Simple { log::debug!("Prolog facts update (Simple mode): marking subscription engine dirty"); *(self_clone.trigger_prolog_subscription_check.lock().await) = true; self_clone.pubsub_publish_diff(diff).await; if let Some(sender) = completion_sender { let _ = sender.send(()); } return; } + + if PROLOG_MODE == PrologMode::SdnaOnly { + let mut diff = diff; + diff.additions.retain(|l| is_sdna_link(&l.data)); + diff.removals.retain(|l| is_sdna_link(&l.data)); + if diff.additions.is_empty() && diff.removals.is_empty() { + if let Some(sender) = completion_sender { let _ = sender.send(()); } + return; + } + // Rebuild using SDNA-only links (e.g., via get_sdna_links_local) + }⛔ Skipped due to learnings
Learnt from: lucksus Repo: coasys/ad4m PR: 644 File: rust-executor/src/perspectives/perspective_instance.rs:2518-2532 Timestamp: 2026-01-15T17:26:10.403Z Learning: In rust-executor/src/perspectives/perspective_instance.rs, the Simple-mode branch of spawn_prolog_facts_update() intentionally publishes diffs so both Prolog subscribers and link-only clients receive updates. Do not remove this publish or “dedupe” it by default; duplicate add/remove events are an accepted trade-off unless explicitly changed later.Learnt from: lucksus Repo: coasys/ad4m PR: 644 File: core/src/perspectives/PerspectiveProxy.ts:1465-1491 Timestamp: 2026-01-15T14:55:56.365Z Learning: The codebase is moving toward making Prolog completely optional. Do not suggest adding Prolog fallbacks or dependencies in code paths that are being migrated to SurrealDB-based implementations (SdnaOnly mode).rust-executor/src/prolog_service/mod.rs (10)
28-52: Clear mode semantics and inline docs.PrologMode variants and the in-file guidance are clear and actionable.
56-81: SimpleEngine encapsulation + initialization look solid.
83-117: Mode guard + pool lookup helpers are straightforward.
119-137: Dirty-flag tracking is consistent with Simple/SdnaOnly behavior.
309-318: SDNA extraction helper is clean and focused.
320-406: Simple/SdnaOnly query paths look consistent and separated.
413-431: Mode short‑circuiting for pooled setup is appropriate.
488-560: Pooled-only routing + diagnostics look good.
563-700: Mode guards and pooled updates are consistent with the new lifecycle.
729-730: Test gating reflects SdnaOnly constraints.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| private generatePropertySetterAction(key: string, metadata: PropertyOptions): any[] { | ||
| if (metadata.setter) { | ||
| // Custom setter - throw error for now (Phase 2) | ||
| throw new Error( | ||
| `Custom setter for property "${key}" not yet supported without Prolog. ` + | ||
| `Use standard @Property decorator or enable Prolog for custom setters.` | ||
| ); | ||
| } | ||
|
|
||
| if (!metadata.through) { | ||
| throw new Error(`Property "${key}" has no 'through' predicate defined`); | ||
| } | ||
|
|
||
| return [{ | ||
| action: "setSingleTarget", | ||
| source: "this", | ||
| predicate: metadata.through, | ||
| target: "value", | ||
| ...(metadata.local && { local: true }) | ||
| }]; |
There was a problem hiding this comment.
Enforce writable=false to protect read‑only properties.
generatePropertySetterAction() ignores the writable flag, so @ReadOnly fields can still be mutated. Please guard this to preserve the contract.
🔧 Proposed fix
private generatePropertySetterAction(key: string, metadata: PropertyOptions): any[] {
if (metadata.setter) {
// Custom setter - throw error for now (Phase 2)
throw new Error(
`Custom setter for property "${key}" not yet supported without Prolog. ` +
`Use standard `@Property` decorator or enable Prolog for custom setters.`
);
}
+ if (metadata.writable === false) {
+ throw new Error(`Property "${key}" is read-only`);
+ }
if (!metadata.through) {
throw new Error(`Property "${key}" has no 'through' predicate defined`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private generatePropertySetterAction(key: string, metadata: PropertyOptions): any[] { | |
| if (metadata.setter) { | |
| // Custom setter - throw error for now (Phase 2) | |
| throw new Error( | |
| `Custom setter for property "${key}" not yet supported without Prolog. ` + | |
| `Use standard @Property decorator or enable Prolog for custom setters.` | |
| ); | |
| } | |
| if (!metadata.through) { | |
| throw new Error(`Property "${key}" has no 'through' predicate defined`); | |
| } | |
| return [{ | |
| action: "setSingleTarget", | |
| source: "this", | |
| predicate: metadata.through, | |
| target: "value", | |
| ...(metadata.local && { local: true }) | |
| }]; | |
| private generatePropertySetterAction(key: string, metadata: PropertyOptions): any[] { | |
| if (metadata.setter) { | |
| // Custom setter - throw error for now (Phase 2) | |
| throw new Error( | |
| `Custom setter for property "${key}" not yet supported without Prolog. ` + | |
| `Use standard `@Property` decorator or enable Prolog for custom setters.` | |
| ); | |
| } | |
| if (metadata.writable === false) { | |
| throw new Error(`Property "${key}" is read-only`); | |
| } | |
| if (!metadata.through) { | |
| throw new Error(`Property "${key}" has no 'through' predicate defined`); | |
| } | |
| return [{ | |
| action: "setSingleTarget", | |
| source: "this", | |
| predicate: metadata.through, | |
| target: "value", | |
| ...(metadata.local && { local: true }) | |
| }]; | |
| } |
🤖 Prompt for AI Agents
In `@core/src/model/Ad4mModel.ts` around lines 641 - 660, In
generatePropertySetterAction, add a guard that checks metadata.writable (or
metadata.readOnly flag if used) and reject setter generation for read-only
properties: if metadata.writable === false then throw a clear Error like
`Property "${key}" is read-only and cannot be written` (place this check before
returning the action and before any custom setter handling so `@ReadOnly` fields
are protected); keep the existing checks for metadata.setter and
metadata.through and preserve the existing action shape when writable is
allowed.
| // Query for all links from this specific node (base expression) | ||
| // Using formatSurrealValue to prevent SQL injection by properly escaping the value | ||
| const safeBaseExpression = ctor.formatSurrealValue(this.#baseExpression); | ||
| const linksQuery = ` | ||
| SELECT id, predicate, out.uri AS target, author, timestamp | ||
| FROM link | ||
| WHERE in.uri = ${safeBaseExpression} | ||
| ORDER BY timestamp ASC | ||
| `; | ||
| const links = await this.#perspective.querySurrealDB(linksQuery); |
There was a problem hiding this comment.
Missing perspective filter in getData() SurrealDB query.
The link query doesn’t constrain by perspective, so it can mix links across perspectives. Add the same perspective filter used elsewhere (e.g., perspective = $perspective).
🔧 Proposed fix
const linksQuery = `
SELECT id, predicate, out.uri AS target, author, timestamp
FROM link
- WHERE in.uri = ${safeBaseExpression}
+ WHERE in.uri = ${safeBaseExpression}
+ AND perspective = $perspective
ORDER BY timestamp ASC
`;🤖 Prompt for AI Agents
In `@core/src/model/Ad4mModel.ts` around lines 752 - 761, The SurrealDB query in
getData() (linksQuery) is missing a perspective constraint, allowing links from
other perspectives; update the WHERE clause in the linksQuery (constructed with
safeBaseExpression via ctor.formatSurrealValue) to also filter by the current
perspective (this.#perspective) — use the same approach you use elsewhere
(either bind a $perspective parameter or escape this.#perspective with
formatSurrealValue) so the query only returns rows where perspective equals the
current perspective before calling this.#perspective.querySurrealDB.
| const matching = links.filter((l: any) => l.predicate === propMeta.predicate); | ||
| if (matching.length > 0) { | ||
| const link = matching[0]; // Take first/latest | ||
| let value = link.target; |
There was a problem hiding this comment.
Property selection uses the oldest link, not the latest.
Links are ordered ASC by timestamp, but matching[0] picks the earliest value. If the intent is “latest wins,” select the last item (or order DESC).
🔧 Proposed fix
- const link = matching[0]; // Take first/latest
+ const link = matching[matching.length - 1]; // Take latest (links are ASC)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const matching = links.filter((l: any) => l.predicate === propMeta.predicate); | |
| if (matching.length > 0) { | |
| const link = matching[0]; // Take first/latest | |
| let value = link.target; | |
| const matching = links.filter((l: any) => l.predicate === propMeta.predicate); | |
| if (matching.length > 0) { | |
| const link = matching[matching.length - 1]; // Take latest (links are ASC) | |
| let value = link.target; |
🤖 Prompt for AI Agents
In `@core/src/model/Ad4mModel.ts` around lines 769 - 772, The code currently
selects the first matching link with matching[0], which picks the oldest link
because links are ordered ASC; change this to pick the latest link by using the
last element (e.g., matching[matching.length - 1]) or reverse/order DESC before
selecting. Update the selection in the block that filters links by
propMeta.predicate (variables: links, matching, propMeta.predicate, link) so the
"latest wins" semantics are enforced.
| // Phase 1: Use metadata instead of Prolog queries | ||
| const metadata = this.getCollectionMetadata(key); | ||
| if (!metadata) { | ||
| console.warn(`Collection "${key}" has no metadata, skipping`); | ||
| return; | ||
| } | ||
|
|
||
| // Generate actions from metadata (replaces Prolog query) | ||
| const actions = this.generateCollectionAction(key, 'setter'); | ||
|
|
||
| if (value) { | ||
| if (Array.isArray(value)) { | ||
| await this.#perspective.executeAction( | ||
| actions, | ||
| this.#baseExpression, | ||
| value.map((v) => ({ name: "value", value: v })), | ||
| batchId | ||
| ); | ||
| } else { | ||
| await this.#perspective.executeAction(actions, this.#baseExpression, [{ name: "value", value }], batchId); | ||
| } |
There was a problem hiding this comment.
Truthy checks drop valid collection values (e.g., 0, false, "").
Using if (value) skips legitimate falsy values in setter/adder/remover. Prefer a nullish check.
🔧 Proposed fix
- if (value) {
+ if (value !== undefined && value !== null) {
if (Array.isArray(value)) {
await this.#perspective.executeAction(
actions,
this.#baseExpression,
value.map((v) => ({ name: "value", value: v })),
batchId
);
} else {
await this.#perspective.executeAction(actions, this.#baseExpression, [{ name: "value", value }], batchId);
}
}Also applies to: 2018-2037, 2042-2061
🤖 Prompt for AI Agents
In `@core/src/model/Ad4mModel.ts` around lines 1993 - 2013, The current truthy
checks around setter/adder/remover logic (using if (value)) in Ad4mModel.ts drop
valid falsy values like 0, false, and ""; update the conditional to a nullish
check (value !== null && value !== undefined or value != null) wherever you call
generateCollectionAction and then call this.#perspective.executeAction
(referenced symbols: getCollectionMetadata, generateCollectionAction,
this.#perspective.executeAction, this.#baseExpression) so that legitimate falsy
values are passed through; apply the same change to the other occurrences noted
(lines around 2018-2037 and 2042-2061).
| } else if (Array.isArray(value)) { | ||
| // Handle all arrays as collections, even empty ones | ||
| if (value.length > 0) { | ||
| await this.setCollectionSetter(key, value, batchId); | ||
| } | ||
| // Skip empty arrays - don't try to set them as properties | ||
| } else if (value !== undefined && value !== null && value !== "") { | ||
| if (setProperties) { | ||
| // Check if this is a collection property (has collection metadata) | ||
| const collMetadata = this.getCollectionMetadata(key); | ||
| if (collMetadata) { | ||
| // Skip - it's a collection, not a regular property | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Empty arrays can’t clear a collection.
innerUpdate() skips empty arrays, so a user can’t clear a collection by setting []. Allow empty arrays to flow into the collection setter (and ensure the action clears existing links).
🔧 Proposed fix
- } else if (Array.isArray(value)) {
- // Handle all arrays as collections, even empty ones
- if (value.length > 0) {
- await this.setCollectionSetter(key, value, batchId);
- }
- // Skip empty arrays - don't try to set them as properties
+ } else if (Array.isArray(value)) {
+ // Treat arrays as collections; allow empty array to clear
+ await this.setCollectionSetter(key, value, batchId);
} else if (value !== undefined && value !== null && value !== "") {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if (Array.isArray(value)) { | |
| // Handle all arrays as collections, even empty ones | |
| if (value.length > 0) { | |
| await this.setCollectionSetter(key, value, batchId); | |
| } | |
| // Skip empty arrays - don't try to set them as properties | |
| } else if (value !== undefined && value !== null && value !== "") { | |
| if (setProperties) { | |
| // Check if this is a collection property (has collection metadata) | |
| const collMetadata = this.getCollectionMetadata(key); | |
| if (collMetadata) { | |
| // Skip - it's a collection, not a regular property | |
| continue; | |
| } | |
| } else if (Array.isArray(value)) { | |
| // Treat arrays as collections; allow empty array to clear | |
| await this.setCollectionSetter(key, value, batchId); | |
| } else if (value !== undefined && value !== null && value !== "") { | |
| if (setProperties) { | |
| // Check if this is a collection property (has collection metadata) | |
| const collMetadata = this.getCollectionMetadata(key); | |
| if (collMetadata) { | |
| // Skip - it's a collection, not a regular property | |
| continue; | |
| } |
🤖 Prompt for AI Agents
In `@core/src/model/Ad4mModel.ts` around lines 2163 - 2176, innerUpdate currently
skips empty arrays so calling setCollectionSetter(key, value, batchId) only for
non-empty arrays prevents clearing collections by assigning []; change
innerUpdate so Array.isArray(value) always calls setCollectionSetter (remove the
value.length check) and ensure setCollectionSetter handles an empty array by
removing/clearing existing collection links for the given key (so setting []
clears the collection). Update references in innerUpdate, setCollectionSetter,
and any logic that relies on getCollectionMetadata to treat arrays as
collections consistently.
| pub fn get_links_by_predicate( | ||
| &self, | ||
| perspective_uuid: &str, | ||
| predicate: &str, | ||
| ) -> Ad4mDbResult<Vec<(LinkExpression, LinkStatus)>> { | ||
| let mut stmt = self.conn.prepare( | ||
| "SELECT perspective, source, predicate, target, author, timestamp, signature, key, status FROM link WHERE perspective = ?1 AND predicate = ?2 ORDER BY timestamp, source, target, author", | ||
| )?; | ||
| let link_iter = stmt.query_map(params![perspective_uuid, predicate], |row| { | ||
| let status: LinkStatus = | ||
| serde_json::from_str(&row.get::<_, String>(8)?).map_err(|e| { | ||
| rusqlite::Error::FromSqlConversionFailure( | ||
| 8, | ||
| rusqlite::types::Type::Text, | ||
| Box::new(e), | ||
| ) | ||
| })?; | ||
| let link_expression = LinkExpression { | ||
| data: Link { | ||
| source: row.get(1)?, | ||
| predicate: row.get(2)?, | ||
| target: row.get(3)?, | ||
| }, | ||
| proof: ExpressionProof { | ||
| signature: row.get(6)?, | ||
| key: row.get(7)?, | ||
| }, | ||
| author: row.get(4)?, | ||
| timestamp: row.get(5)?, | ||
| status: Some(status.clone()), | ||
| }; | ||
| Ok((link_expression, status)) | ||
| })?; | ||
| let links: Result<Vec<_>, _> = link_iter.collect(); | ||
| Ok(links?) | ||
| } |
There was a problem hiding this comment.
Normalize empty predicate to None for consistency.
add_link() stores missing predicates as "", and get_link() maps "" → None, but this new method returns Some(""). That inconsistency can leak the sentinel and break downstream logic that expects None. Consider normalizing here (and ideally in a shared helper).
♻️ Suggested fix
- data: Link {
- source: row.get(1)?,
- predicate: row.get(2)?,
- target: row.get(3)?,
- },
+ data: Link {
+ source: row.get(1)?,
+ predicate: row
+ .get::<_, Option<String>>(2)?
+ .and_then(|p| if p.is_empty() { None } else { Some(p) }),
+ target: row.get(3)?,
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn get_links_by_predicate( | |
| &self, | |
| perspective_uuid: &str, | |
| predicate: &str, | |
| ) -> Ad4mDbResult<Vec<(LinkExpression, LinkStatus)>> { | |
| let mut stmt = self.conn.prepare( | |
| "SELECT perspective, source, predicate, target, author, timestamp, signature, key, status FROM link WHERE perspective = ?1 AND predicate = ?2 ORDER BY timestamp, source, target, author", | |
| )?; | |
| let link_iter = stmt.query_map(params![perspective_uuid, predicate], |row| { | |
| let status: LinkStatus = | |
| serde_json::from_str(&row.get::<_, String>(8)?).map_err(|e| { | |
| rusqlite::Error::FromSqlConversionFailure( | |
| 8, | |
| rusqlite::types::Type::Text, | |
| Box::new(e), | |
| ) | |
| })?; | |
| let link_expression = LinkExpression { | |
| data: Link { | |
| source: row.get(1)?, | |
| predicate: row.get(2)?, | |
| target: row.get(3)?, | |
| }, | |
| proof: ExpressionProof { | |
| signature: row.get(6)?, | |
| key: row.get(7)?, | |
| }, | |
| author: row.get(4)?, | |
| timestamp: row.get(5)?, | |
| status: Some(status.clone()), | |
| }; | |
| Ok((link_expression, status)) | |
| })?; | |
| let links: Result<Vec<_>, _> = link_iter.collect(); | |
| Ok(links?) | |
| } | |
| pub fn get_links_by_predicate( | |
| &self, | |
| perspective_uuid: &str, | |
| predicate: &str, | |
| ) -> Ad4mDbResult<Vec<(LinkExpression, LinkStatus)>> { | |
| let mut stmt = self.conn.prepare( | |
| "SELECT perspective, source, predicate, target, author, timestamp, signature, key, status FROM link WHERE perspective = ?1 AND predicate = ?2 ORDER BY timestamp, source, target, author", | |
| )?; | |
| let link_iter = stmt.query_map(params![perspective_uuid, predicate], |row| { | |
| let status: LinkStatus = | |
| serde_json::from_str(&row.get::<_, String>(8)?).map_err(|e| { | |
| rusqlite::Error::FromSqlConversionFailure( | |
| 8, | |
| rusqlite::types::Type::Text, | |
| Box::new(e), | |
| ) | |
| })?; | |
| let link_expression = LinkExpression { | |
| data: Link { | |
| source: row.get(1)?, | |
| predicate: row | |
| .get::<_, Option<String>>(2)? | |
| .and_then(|p| if p.is_empty() { None } else { Some(p) }), | |
| target: row.get(3)?, | |
| }, | |
| proof: ExpressionProof { | |
| signature: row.get(6)?, | |
| key: row.get(7)?, | |
| }, | |
| author: row.get(4)?, | |
| timestamp: row.get(5)?, | |
| status: Some(status.clone()), | |
| }; | |
| Ok((link_expression, status)) | |
| })?; | |
| let links: Result<Vec<_>, _> = link_iter.collect(); | |
| Ok(links?) | |
| } |
🤖 Prompt for AI Agents
In `@rust-executor/src/db.rs` around lines 1176 - 1211, get_links_by_predicate
currently returns LinkExpression.data.predicate as Some("") when the DB stored
an empty sentinel, causing inconsistency with add_link/get_link which map "" →
None; update get_links_by_predicate to normalize the predicate value by reading
the raw string from row.get(2) and converting an empty string to None (i.e., set
Link.data.predicate = None when the retrieved predicate == ""), and consider
factoring this normalization into a shared helper used by add_link, get_link,
and get_links_by_predicate for consistency.
| /// Helper to mark the Prolog engine as dirty (needs update before next query) | ||
| /// Only applies to Simple/SdnaOnly modes | ||
| async fn mark_prolog_engine_dirty(&self) { | ||
| if PROLOG_MODE == PrologMode::Simple { | ||
| let perspective_uuid = self.persisted.lock().await.uuid.clone(); | ||
| get_prolog_service() | ||
| .await | ||
| .mark_dirty(&perspective_uuid) | ||
| .await; | ||
| } | ||
| } | ||
|
|
||
| /// Combined helper: spawns Prolog facts update AND marks query engine as dirty | ||
| /// This is the common pattern throughout the codebase | ||
| async fn update_prolog_engines(&self, diff: DecoratedPerspectiveDiff) { | ||
| // Update subscription engine (immediate via spawned task) | ||
| self.spawn_prolog_facts_update(diff, None); | ||
|
|
||
| // Mark query engine dirty for lazy update on next query | ||
| self.mark_prolog_engine_dirty().await; | ||
| } |
There was a problem hiding this comment.
Align SdnaOnly handling with the helper’s doc comment.
The comment says “Simple/SdnaOnly,” but the implementation only marks dirty for Simple. Either expand to include SdnaOnly or update the comment to avoid confusion.
🛠️ Possible alignment
- if PROLOG_MODE == PrologMode::Simple {
+ if matches!(PROLOG_MODE, PrologMode::Simple | PrologMode::SdnaOnly) {
let perspective_uuid = self.persisted.lock().await.uuid.clone();
get_prolog_service()
.await
.mark_dirty(&perspective_uuid)
.await;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Helper to mark the Prolog engine as dirty (needs update before next query) | |
| /// Only applies to Simple/SdnaOnly modes | |
| async fn mark_prolog_engine_dirty(&self) { | |
| if PROLOG_MODE == PrologMode::Simple { | |
| let perspective_uuid = self.persisted.lock().await.uuid.clone(); | |
| get_prolog_service() | |
| .await | |
| .mark_dirty(&perspective_uuid) | |
| .await; | |
| } | |
| } | |
| /// Combined helper: spawns Prolog facts update AND marks query engine as dirty | |
| /// This is the common pattern throughout the codebase | |
| async fn update_prolog_engines(&self, diff: DecoratedPerspectiveDiff) { | |
| // Update subscription engine (immediate via spawned task) | |
| self.spawn_prolog_facts_update(diff, None); | |
| // Mark query engine dirty for lazy update on next query | |
| self.mark_prolog_engine_dirty().await; | |
| } | |
| /// Helper to mark the Prolog engine as dirty (needs update before next query) | |
| /// Only applies to Simple/SdnaOnly modes | |
| async fn mark_prolog_engine_dirty(&self) { | |
| if matches!(PROLOG_MODE, PrologMode::Simple | PrologMode::SdnaOnly) { | |
| let perspective_uuid = self.persisted.lock().await.uuid.clone(); | |
| get_prolog_service() | |
| .await | |
| .mark_dirty(&perspective_uuid) | |
| .await; | |
| } | |
| } | |
| /// Combined helper: spawns Prolog facts update AND marks query engine as dirty | |
| /// This is the common pattern throughout the codebase | |
| async fn update_prolog_engines(&self, diff: DecoratedPerspectiveDiff) { | |
| // Update subscription engine (immediate via spawned task) | |
| self.spawn_prolog_facts_update(diff, None); | |
| // Mark query engine dirty for lazy update on next query | |
| self.mark_prolog_engine_dirty().await; | |
| } |
🤖 Prompt for AI Agents
In `@rust-executor/src/perspectives/perspective_instance.rs` around lines 1800 -
1820, The doc for mark_prolog_engine_dirty says it applies to "Simple/SdnaOnly"
but the implementation only checks PrologMode::Simple; either update the runtime
check to include SdnaOnly (e.g., check PROLOG_MODE == PrologMode::Simple ||
PROLOG_MODE == PrologMode::SdnaOnly) or change the doc comment to remove
SdnaOnly to match current behavior; adjust the check in the
mark_prolog_engine_dirty method (and ensure any callers like
update_prolog_engines keep expected semantics) and reference the PrologMode enum
and PROLOG_MODE global to locate where to change.
| // LOCK SCOPE OPTIMIZATION: Acquire write lock ONLY to check state, then release | ||
| let (needs_update, engine_exists) = { | ||
| let engines = self.simple_engines.read().await; | ||
|
|
||
| // Check if we need to update (dirty or links changed or first time) | ||
| let needs_update = if PROLOG_MODE == PrologMode::SdnaOnly { | ||
| // In SdnaOnly mode, only update if SDNA links actually changed | ||
| if let Some(simple_engine) = engines.get(perspective_id) { | ||
| if simple_engine.dirty { | ||
| true | ||
| } else if let Some(ref current_sdna) = simple_engine.current_sdna_links { | ||
| // Extract current SDNA links and compare | ||
| let new_sdna_links = Self::extract_sdna_links(links); | ||
| current_sdna != &new_sdna_links | ||
| } else { | ||
| true // No SDNA tracking yet, need init | ||
| } | ||
| } else { | ||
| true // First query = needs init | ||
| } | ||
| } else if let Some(simple_engine) = engines.get(perspective_id) { | ||
| simple_engine.dirty || simple_engine.current_links != links | ||
| } else { | ||
| true // First query = needs init | ||
| }; | ||
|
|
||
| let engine_exists = engines.contains_key(perspective_id); | ||
| (needs_update, engine_exists) | ||
| }; // Read lock released here | ||
|
|
||
| if needs_update { | ||
| let mode_desc = match PROLOG_MODE { | ||
| PrologMode::SdnaOnly => "SDNA-only mode (no link data)", | ||
| _ => "Simple mode: lazy update", | ||
| }; | ||
| log::debug!( | ||
| "Updating Prolog engine {} ({} with {} links)", | ||
| perspective_id, | ||
| mode_desc, | ||
| links.len() | ||
| ); | ||
|
|
||
| // EXPENSIVE OPERATIONS OUTSIDE THE LOCK: | ||
| // Create and spawn engines if they don't exist (BEFORE acquiring write lock) | ||
| let (query_engine, subscription_engine) = if !engine_exists { | ||
| let mut qe = PrologEngine::new(); | ||
| qe.spawn().await?; // Expensive async operation - no lock held | ||
|
|
||
| let mut se = PrologEngine::new(); | ||
| se.spawn().await?; // Expensive async operation - no lock held | ||
|
|
||
| (qe, se) | ||
| } else { | ||
| // Engines exist, we'll update them - create placeholders for now | ||
| (PrologEngine::new(), PrologEngine::new()) | ||
| }; | ||
|
|
||
| // Prepare facts based on mode (no lock needed - just data preparation) | ||
| let mut facts_to_load = get_static_infrastructure_facts(); | ||
|
|
||
| // Only load link data if not in SDNA-only mode | ||
| if PROLOG_MODE != PrologMode::SdnaOnly { | ||
| facts_to_load.extend(get_data_facts(links)); | ||
| } | ||
|
|
||
| // Always load SDNA facts | ||
| facts_to_load.extend(get_sdna_facts( | ||
| links, | ||
| neighbourhood_author.clone(), | ||
| owner_did.clone(), | ||
| )?); | ||
|
|
||
| // Preprocess facts (handle embeddings) - EXPENSIVE, no lock held | ||
| let embedding_cache = Arc::new(RwLock::new(EmbeddingCache::new())); | ||
| let processed_facts = | ||
| PoolUtils::preprocess_program_lines(facts_to_load, &embedding_cache).await; | ||
|
|
||
| // LOCK SCOPE: Acquire write lock ONLY to get mutable engine references | ||
| let mut engines = self.simple_engines.write().await; | ||
|
|
||
| // Insert new engines if needed | ||
| if !engine_exists { | ||
| engines.insert( | ||
| perspective_id.to_string(), | ||
| SimpleEngine { | ||
| query_engine, | ||
| subscription_engine, | ||
| dirty: true, | ||
| current_links: Vec::new(), | ||
| current_sdna_links: None, | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| // Get mutable reference and move engines out temporarily | ||
| let simple_engine = engines.get_mut(perspective_id).unwrap(); | ||
|
|
||
| // Move engines out of the struct temporarily | ||
| let query_engine_to_update = | ||
| std::mem::replace(&mut simple_engine.query_engine, PrologEngine::new()); | ||
| let subscription_engine_to_update = | ||
| std::mem::replace(&mut simple_engine.subscription_engine, PrologEngine::new()); | ||
|
|
||
| // Release write lock before expensive load operations | ||
| drop(engines); | ||
|
|
||
| // EXPENSIVE OPERATIONS OUTSIDE THE LOCK: | ||
| // Load facts into both engines | ||
| query_engine_to_update | ||
| .load_module_string("facts", &processed_facts) | ||
| .await?; | ||
| subscription_engine_to_update | ||
| .load_module_string("facts", &processed_facts) | ||
| .await?; | ||
|
|
||
| // LOCK SCOPE: Reacquire write lock to update final state | ||
| let mut engines = self.simple_engines.write().await; | ||
| let simple_engine = engines.get_mut(perspective_id).unwrap(); | ||
|
|
||
| // Move engines back | ||
| simple_engine.query_engine = query_engine_to_update; | ||
| simple_engine.subscription_engine = subscription_engine_to_update; | ||
|
|
||
| simple_engine.dirty = false; | ||
|
|
||
| // MEMORY OPTIMIZATION: In SdnaOnly mode, don't store full links | ||
| if PROLOG_MODE == PrologMode::SdnaOnly { | ||
| simple_engine.current_links = Vec::new(); // Empty - not needed in SdnaOnly mode | ||
| simple_engine.current_sdna_links = Some(Self::extract_sdna_links(links)); | ||
| } else { | ||
| simple_engine.current_links = links.to_vec(); | ||
| } | ||
|
|
||
| log::debug!( | ||
| "Prolog engines {} updated successfully (query + subscription)", | ||
| perspective_id | ||
| ); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Restore engines on load failure to avoid placeholder corruption.
At Line 265–280, engines are swapped out and replaced with PrologEngine::new(). If load_module_string() fails, the function exits early and the map is left with unspawned placeholder engines, so subsequent queries will run against empty engines. Please restore the original engines (and keep dirty = true) on error.
🛠️ Proposed fix (error-path restore)
- // EXPENSIVE OPERATIONS OUTSIDE THE LOCK:
- // Load facts into both engines
- query_engine_to_update
- .load_module_string("facts", &processed_facts)
- .await?;
- subscription_engine_to_update
- .load_module_string("facts", &processed_facts)
- .await?;
+ // EXPENSIVE OPERATIONS OUTSIDE THE LOCK:
+ // Load facts into both engines, restoring on failure
+ let load_result: Result<(), Error> = async {
+ query_engine_to_update
+ .load_module_string("facts", &processed_facts)
+ .await?;
+ subscription_engine_to_update
+ .load_module_string("facts", &processed_facts)
+ .await?;
+ Ok(())
+ }
+ .await;
+
+ if let Err(e) = load_result {
+ let mut engines = self.simple_engines.write().await;
+ if let Some(simple_engine) = engines.get_mut(perspective_id) {
+ simple_engine.query_engine = query_engine_to_update;
+ simple_engine.subscription_engine = subscription_engine_to_update;
+ simple_engine.dirty = true;
+ }
+ return Err(e);
+ }🤖 Prompt for AI Agents
In `@rust-executor/src/prolog_service/mod.rs` around lines 167 - 307, The map is
left with unspawned placeholder engines if load_module_string() fails after
swapping engines out; wrap the expensive load operations for
query_engine_to_update and subscription_engine_to_update in error-handling that
on any Err reacquires self.simple_engines.write().await and restores the
original engines into the SimpleEngine (assign simple_engine.query_engine =
query_engine_to_update and simple_engine.subscription_engine =
subscription_engine_to_update), set simple_engine.dirty = true (so update will
be retried), then return the error; use the existing symbols (simple_engines,
SimpleEngine, query_engine_to_update, subscription_engine_to_update,
load_module_string) and avoid leaving PrologEngine::new() placeholders in the
map on failure.
Memory Optimization: SDNA-Only Prolog + Whisper Sharing
Problem
AD4M was experiencing excessive memory usage (up to 84GB with 2 users), causing system slowdowns on macOS and crashes on Linux. Investigation revealed multiple memory accumulation issues rather than traditional memory leaks.
Root Causes Identified
🔥 CRITICAL 1: Prolog Engines with Full Link Data (Potential 20-50GB!)
rust-executor/src/prolog_service/mod.rs🔥 CRITICAL 2: AI Service - Whisper Model Per Stream (Potential 10-30GB!)
rust-executor/src/ai_service/mod.rs:11111. Massive Query Result Cloning (CRITICAL)
rust-executor/src/perspectives/perspective_instance.rs:3397&3324last_resultstrings2. Slow Filtered Pool Cleanup
rust-executor/src/prolog_service/engine_pool.rs:45-463. Long Subscription Timeouts
rust-executor/src/perspectives/perspective_instance.rs:534. Unbounded Subscription Result Storage
Solutions Implemented
🔥 Fix 1: SDNA-Only Prolog Mode (BIGGEST WIN! 20-50GB SAVED!)
Files Modified:
rust-executor/src/prolog_service/mod.rsrust-executor/src/perspectives/sdna.rscore/src/perspectives/PerspectiveProxy.tscore/src/model/Subject.tsThe Problem: Prolog engines loaded ALL perspective links as facts, consuming 20-50GB with multiple perspectives
The Solution: Prolog engines now run in SdnaOnly mode - they ONLY contain SDNA subject class definitions, NOT link data. All instance queries go through SurrealDB instead.
How It Works:
Rust Side (
rust-executor/src/prolog_service/mod.rs):TypeScript Side - Changed query strategy:
Instance Detection (
PerspectiveProxy.ts:isSubjectInstance()):@Flag({through: "ad4m://type", value: "ad4m://message"}):Instance Discovery (
PerspectiveProxy.ts:getAllSubjectInstances()):Property Access (
Subject.ts:getPropertyValue(),getCollectionValues()):where: { isInstance: Message }) applied viaisSubjectInstance()checksCollection Filters (
PerspectiveProxy.ts:getCollectionValuesViaSurreal()):instanceFilterfrom SDNA'ssubject_class("ClassName", ...)patternsWhat Prolog IS Used For:
What Prolog is NOT Used For:
Implementation Details:
triple(Base, "predicate", "exact_target")patternsinstance/2definitionsubject_class("ClassName", OtherClass)in collection_getterImpact:
🔥 Fix 2: Arc-Based Whisper Model Sharing (SECOND BIGGEST WIN!)
Files Modified:
rust-executor/src/ai_service/mod.rsThe Problem: Every
open_transcription_stream()call loaded a NEW 500MB-1.5GB Whisper model into vRAMThe Solution: Load ONE model, share it via
Arc<Whisper>across ALL transcription streamsHow It Works:
Arc<Mutex<Option<Arc<Whisper>>>>Arc(just increments reference count - cheap!)Implementation:
Impact:
Fix 3: Aggressive Transcription Cleanup
Files Modified:
rust-executor/src/ai_service/mod.rsChanges:
Impact:
Fix 4: Remove Subscription Result Truncation
Files Modified:
rust-executor/src/perspectives/perspective_instance.rsThe Problem: Subscription results were being truncated to 100KB, which was breaking results for legitimate large queries
The Solution: Removed truncation entirely - subscription results are no longer artificially limited
Changes:
MAX_SUBSCRIPTION_RESULT_SIZEconstanttruncate_subscription_result()functionImpact: Subscriptions with large results now work correctly
Fix 5: Eliminate Excessive Cloning in Subscription Loops
Files Modified:
rust-executor/src/perspectives/perspective_instance.rsBefore:
After:
Impact: If 100 subscriptions with 1MB
last_resulteach checked every 200ms:Fix 6: Reduce Subscription Timeout
Files Modified:
rust-executor/src/perspectives/perspective_instance.rsChanges:
Impact: 5x faster cleanup of stale subscriptions
Fix #7: Aggressive Filtered Pool Cleanup
Files Modified:
rust-executor/src/prolog_service/engine_pool.rsChanges:
Impact: 7.5x faster cleanup prevents accumulation (Note: Less relevant in SdnaOnly mode since pools aren't used)
Memory Usage Breakdown - Before vs After
Before Fixes (74GB with 2 users, heavy Flux):
After Fixes (Expected ~5-10GB with 2 users):
Total Expected Savings: 42-95GB (85-95% reduction!)
Expected Results
With these fixes, memory usage should:
SDNA-Only Mode Benefits:
Key Technical Insights
Why SDNA-Only Mode Works:
Why Arc-based Whisper Sharing Works:
Arc<Whisper>doesn't duplicate weights in vRAM/RAMTesting Recommendations
SDNA/Subject Class Testing:
npm test -- prolog-and-literals- All 78 tests should passisSubjectInstance()works with flags and required propertieswhere: { isInstance: ClassName }works correctlyAI Service Testing:
"Loading shared Whisper model (ONE model for ALL streams)...""Opening transcription stream (reusing shared Whisper model)"General Monitoring:
htop(Linux)Migration Notes
Prolog Service Changes (MAJOR - BREAKING):
@Property,@Collection,@Flag, etc.) work exactly as beforeAI Service Changes:
Architecture Change: Whisper model now shared across all transcription streams
Timeout Reduction: Transcription streams timeout in 30 seconds (was 2 minutes)
Subscription Changes:
Ready for testing! Expected memory reduction: 85-95% 🚀
Key wins:
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.